From 0b14b654b6e5d936f7241dcc0c249e0d4cc42728 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 23 Jan 2017 18:40:25 -0200 Subject: [PATCH] Gather issuable metadata to avoid n+ queries on index view --- .../concerns/issuable_collections.rb | 22 +++++++ app/controllers/concerns/issues_action.rb | 3 + .../concerns/merge_requests_action.rb | 3 + app/controllers/projects/issues_controller.rb | 7 ++- .../projects/merge_requests_controller.rb | 7 ++- app/models/award_emoji.rb | 8 +++ app/models/concerns/issuable.rb | 5 ++ app/models/note.rb | 6 ++ app/views/projects/issues/_issue.html.haml | 17 +----- .../merge_requests/_merge_request.html.haml | 17 +----- .../shared/_issuable_meta_data.html.haml | 19 +++++++ changelogs/unreleased/issue_25900.yml | 4 ++ spec/controllers/dashboard_controller_spec.rb | 19 +++++++ .../projects/issues_controller_spec.rb | 2 + .../merge_requests_controller_spec.rb | 4 ++ spec/features/issuables/issuable_list_spec.rb | 57 +++++++++++++++++++ ...issuables_list_metadata_shared_examples.rb | 35 ++++++++++++ 17 files changed, 199 insertions(+), 36 deletions(-) create mode 100644 app/views/shared/_issuable_meta_data.html.haml create mode 100644 changelogs/unreleased/issue_25900.yml create mode 100644 spec/controllers/dashboard_controller_spec.rb create mode 100644 spec/features/issuables/issuable_list_spec.rb create mode 100644 spec/support/issuables_list_metadata_shared_examples.rb 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)} · 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)} · 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