From a2f57f23616bc1ab7547e5a7c517cdd49b172bd8 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Mon, 19 Dec 2016 21:26:15 +0000 Subject: [PATCH 1/2] Fix N+1 queries on milestone show pages --- app/models/issue.rb | 2 ++ app/views/shared/milestones/_tabs.html.haml | 2 +- changelogs/unreleased/jej-fix-n-1-queries-milestones-show.yml | 4 ++++ 3 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/jej-fix-n-1-queries-milestones-show.yml diff --git a/app/models/issue.rb b/app/models/issue.rb index 738c96e4db3..6825553512f 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -39,6 +39,8 @@ class Issue < ActiveRecord::Base scope :created_after, -> (datetime) { where("created_at >= ?", datetime) } + scope :include_associations, -> { includes(:assignee, :labels, project: :namespace) } + attr_spammable :title, spam_title: true attr_spammable :description, spam_description: true diff --git a/app/views/shared/milestones/_tabs.html.haml b/app/views/shared/milestones/_tabs.html.haml index 2b6ce2d7e7a..c8f2319d95a 100644 --- a/app/views/shared/milestones/_tabs.html.haml +++ b/app/views/shared/milestones/_tabs.html.haml @@ -21,7 +21,7 @@ .tab-content.milestone-content .tab-pane.active#tab-issues - = render 'shared/milestones/issues_tab', issues: milestone.issues_visible_to_user(current_user), show_project_name: show_project_name, show_full_project_name: show_full_project_name + = render 'shared/milestones/issues_tab', issues: milestone.issues_visible_to_user(current_user).include_associations, show_project_name: show_project_name, show_full_project_name: show_full_project_name .tab-pane#tab-merge-requests = render 'shared/milestones/merge_requests_tab', merge_requests: milestone.merge_requests, show_project_name: show_project_name, show_full_project_name: show_full_project_name .tab-pane#tab-participants diff --git a/changelogs/unreleased/jej-fix-n-1-queries-milestones-show.yml b/changelogs/unreleased/jej-fix-n-1-queries-milestones-show.yml new file mode 100644 index 00000000000..ad6eba3faf2 --- /dev/null +++ b/changelogs/unreleased/jej-fix-n-1-queries-milestones-show.yml @@ -0,0 +1,4 @@ +--- +title: Fix N+1 queries on milestone show pages +merge_request: 8185 +author: From 9c623e3e5d7434f2e30f7c389d13e5af4ede770a Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Tue, 20 Dec 2016 14:48:04 +0000 Subject: [PATCH 2/2] Added QueryRecorder to test N+1 fix on Milestone#show --- spec/features/milestones/show_spec.rb | 26 +++++++++++++++++ spec/support/query_recorder.rb | 40 +++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 spec/features/milestones/show_spec.rb create mode 100644 spec/support/query_recorder.rb diff --git a/spec/features/milestones/show_spec.rb b/spec/features/milestones/show_spec.rb new file mode 100644 index 00000000000..40b4dc63697 --- /dev/null +++ b/spec/features/milestones/show_spec.rb @@ -0,0 +1,26 @@ +require 'rails_helper' + +describe 'Milestone show', feature: true do + let(:user) { create(:user) } + let(:project) { create(:empty_project) } + let(:milestone) { create(:milestone, project: project) } + let(:labels) { create_list(:label, 2, project: project) } + let(:issue_params) { { project: project, assignee: user, author: user, milestone: milestone, labels: labels } } + + before do + project.add_user(user, :developer) + login_as(user) + end + + def visit_milestone + visit namespace_project_milestone_path(project.namespace, project, milestone) + end + + it 'avoids N+1 database queries' do + create(:labeled_issue, issue_params) + control_count = ActiveRecord::QueryRecorder.new { visit_milestone }.count + create_list(:labeled_issue, 10, issue_params) + + expect { visit_milestone }.not_to exceed_query_limit(control_count) + end +end diff --git a/spec/support/query_recorder.rb b/spec/support/query_recorder.rb new file mode 100644 index 00000000000..e40d5ebd9a8 --- /dev/null +++ b/spec/support/query_recorder.rb @@ -0,0 +1,40 @@ +module ActiveRecord + class QueryRecorder + attr_reader :log + + def initialize(&block) + @log = [] + ActiveSupport::Notifications.subscribed(method(:callback), 'sql.active_record', &block) + end + + def callback(name, start, finish, message_id, values) + return if %w(CACHE SCHEMA).include?(values[:name]) + @log << values[:sql] + end + + def count + @log.count + end + + def log_message + @log.join("\n\n") + end + end +end + +RSpec::Matchers.define :exceed_query_limit do |expected| + supports_block_expectations + + match do |block| + query_count(&block) > expected + end + + failure_message_when_negated do |actual| + "Expected a maximum of #{expected} queries, got #{@recorder.count}:\n\n#{@recorder.log_message}" + end + + def query_count(&block) + @recorder = ActiveRecord::QueryRecorder.new(&block) + @recorder.count + end +end