From cbc20d2b7f8c73e2892c0c458619df2a9fe0c9ab Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sun, 3 Jun 2018 03:31:41 -0700 Subject: [PATCH] Remove N+1 query for author in issues API This was being masked by the statement cache because only one author was used per issue in the test.. Also adds support for an Rspec matcher `exceed_all_query_limit`. --- .../sh-add-uncached-query-limiter.yml | 5 ++ doc/development/query_recorder.md | 13 ++++ lib/api/issues.rb | 2 +- spec/requests/api/issues_spec.rb | 8 ++- spec/support/helpers/query_recorder.rb | 7 +- spec/support/matchers/exceed_query_limit.rb | 65 ++++++++++++++----- 6 files changed, 78 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/sh-add-uncached-query-limiter.yml diff --git a/changelogs/unreleased/sh-add-uncached-query-limiter.yml b/changelogs/unreleased/sh-add-uncached-query-limiter.yml new file mode 100644 index 00000000000..4318338c229 --- /dev/null +++ b/changelogs/unreleased/sh-add-uncached-query-limiter.yml @@ -0,0 +1,5 @@ +--- +title: Remove N+1 query for author in issues API +merge_request: +author: +type: performance diff --git a/doc/development/query_recorder.md b/doc/development/query_recorder.md index 26d3355e94d..61e5e1afede 100644 --- a/doc/development/query_recorder.md +++ b/doc/development/query_recorder.md @@ -22,6 +22,19 @@ As an example you might create 5 issues in between counts, which would cause the > **Note:** In some cases the query count might change slightly between runs for unrelated reasons. In this case you might need to test `exceed_query_limit(control_count + acceptable_change)`, but this should be avoided if possible. +## Cached queries + +By default, QueryRecorder will ignore cached queries in the count. However, it may be better to count +all queries to avoid introducing an N+1 query that may be masked by the statement cache. To do this, +pass the `skip_cached` variable to `QueryRecorder` and use the `exceed_all_query_limit` matcher: + +it "avoids N+1 database queries" do + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { visit_some_page }.count + create_list(:issue, 5) + expect { visit_some_page }.not_to exceed_all_query_limit(control_count) +end +``` + ## Finding the source of the query It may be useful to identify the source of the queries by looking at the call backtrace. diff --git a/lib/api/issues.rb b/lib/api/issues.rb index b64f465ce56..25185d6edc8 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -16,7 +16,7 @@ module API args[:scope] = args[:scope].underscore if args[:scope] issues = IssuesFinder.new(current_user, args).execute - .preload(:assignees, :labels, :notes, :timelogs, :project) + .preload(:assignees, :labels, :notes, :timelogs, :project, :author) issues.reorder(args[:order_by] => args[:sort]) end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 4181f4ebbbe..a15d60aafe0 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -630,15 +630,17 @@ describe API::Issues do end it 'avoids N+1 queries' do - control_count = ActiveRecord::QueryRecorder.new do + get api("/projects/#{project.id}/issues", user) + + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do get api("/projects/#{project.id}/issues", user) end.count - create(:issue, author: user, project: project) + create_list(:issue, 3, project: project) expect do get api("/projects/#{project.id}/issues", user) - end.not_to exceed_query_limit(control_count) + end.not_to exceed_all_query_limit(control_count) end it 'returns 404 when project does not exist' do diff --git a/spec/support/helpers/query_recorder.rb b/spec/support/helpers/query_recorder.rb index 28536bbef5e..7ce63375d34 100644 --- a/spec/support/helpers/query_recorder.rb +++ b/spec/support/helpers/query_recorder.rb @@ -1,10 +1,11 @@ module ActiveRecord class QueryRecorder - attr_reader :log, :cached + attr_reader :log, :skip_cached, :cached - def initialize(&block) + def initialize(skip_cached: true, &block) @log = [] @cached = [] + @skip_cached = skip_cached ActiveSupport::Notifications.subscribed(method(:callback), 'sql.active_record', &block) end @@ -16,7 +17,7 @@ module ActiveRecord def callback(name, start, finish, message_id, values) show_backtrace(values) if ENV['QUERY_RECORDER_DEBUG'] - if values[:name]&.include?("CACHE") + if values[:name]&.include?("CACHE") && skip_cached @cached << values[:sql] elsif !values[:name]&.include?("SCHEMA") @log << values[:sql] diff --git a/spec/support/matchers/exceed_query_limit.rb b/spec/support/matchers/exceed_query_limit.rb index 88d22a3ddd9..cd042401f3a 100644 --- a/spec/support/matchers/exceed_query_limit.rb +++ b/spec/support/matchers/exceed_query_limit.rb @@ -1,17 +1,4 @@ -RSpec::Matchers.define :exceed_query_limit do |expected| - supports_block_expectations - - match do |block| - @subject_block = block - actual_count > expected_count + threshold - end - - failure_message_when_negated do |actual| - threshold_message = threshold > 0 ? " (+#{@threshold})" : '' - counts = "#{expected_count}#{threshold_message}" - "Expected a maximum of #{counts} queries, got #{actual_count}:\n\n#{log_message}" - end - +module ExceedQueryLimitHelpers def with_threshold(threshold) @threshold = threshold self @@ -43,7 +30,7 @@ RSpec::Matchers.define :exceed_query_limit do |expected| end def recorder - @recorder ||= ActiveRecord::QueryRecorder.new(&@subject_block) + @recorder ||= ActiveRecord::QueryRecorder.new(skip_cached: skip_cached, &@subject_block) end def count_queries(queries) @@ -61,4 +48,52 @@ RSpec::Matchers.define :exceed_query_limit do |expected| @recorder.log_message end end + + def skip_cached + true + end + + def verify_count(&block) + @subject_block = block + actual_count > expected_count + threshold + end + + def failure_message + threshold_message = threshold > 0 ? " (+#{@threshold})" : '' + counts = "#{expected_count}#{threshold_message}" + "Expected a maximum of #{counts} queries, got #{actual_count}:\n\n#{log_message}" + end +end + +RSpec::Matchers.define :exceed_all_query_limit do |expected| + supports_block_expectations + + include ExceedQueryLimitHelpers + + match do |block| + verify_count(&block) + end + + failure_message_when_negated do |actual| + failure_message + end + + def skip_cached + false + end +end + +# Excludes cached methods from the query count +RSpec::Matchers.define :exceed_query_limit do |expected| + supports_block_expectations + + include ExceedQueryLimitHelpers + + match do |block| + verify_count(&block) + end + + failure_message_when_negated do |actual| + failure_message + end end