Commit cbc20d2b authored by Stan Hu's avatar Stan Hu

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`.
parent fe0ebf76
---
title: Remove N+1 query for author in issues API
merge_request:
author:
type: performance
...@@ -22,6 +22,19 @@ As an example you might create 5 issues in between counts, which would cause the ...@@ -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. > **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 ## Finding the source of the query
It may be useful to identify the source of the queries by looking at the call backtrace. It may be useful to identify the source of the queries by looking at the call backtrace.
......
...@@ -16,7 +16,7 @@ module API ...@@ -16,7 +16,7 @@ module API
args[:scope] = args[:scope].underscore if args[:scope] args[:scope] = args[:scope].underscore if args[:scope]
issues = IssuesFinder.new(current_user, args).execute 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]) issues.reorder(args[:order_by] => args[:sort])
end end
......
...@@ -630,15 +630,17 @@ describe API::Issues do ...@@ -630,15 +630,17 @@ describe API::Issues do
end end
it 'avoids N+1 queries' do 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) get api("/projects/#{project.id}/issues", user)
end.count end.count
create(:issue, author: user, project: project) create_list(:issue, 3, project: project)
expect do expect do
get api("/projects/#{project.id}/issues", user) 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 end
it 'returns 404 when project does not exist' do it 'returns 404 when project does not exist' do
......
module ActiveRecord module ActiveRecord
class QueryRecorder class QueryRecorder
attr_reader :log, :cached attr_reader :log, :skip_cached, :cached
def initialize(&block) def initialize(skip_cached: true, &block)
@log = [] @log = []
@cached = [] @cached = []
@skip_cached = skip_cached
ActiveSupport::Notifications.subscribed(method(:callback), 'sql.active_record', &block) ActiveSupport::Notifications.subscribed(method(:callback), 'sql.active_record', &block)
end end
...@@ -16,7 +17,7 @@ module ActiveRecord ...@@ -16,7 +17,7 @@ module ActiveRecord
def callback(name, start, finish, message_id, values) def callback(name, start, finish, message_id, values)
show_backtrace(values) if ENV['QUERY_RECORDER_DEBUG'] show_backtrace(values) if ENV['QUERY_RECORDER_DEBUG']
if values[:name]&.include?("CACHE") if values[:name]&.include?("CACHE") && skip_cached
@cached << values[:sql] @cached << values[:sql]
elsif !values[:name]&.include?("SCHEMA") elsif !values[:name]&.include?("SCHEMA")
@log << values[:sql] @log << values[:sql]
......
RSpec::Matchers.define :exceed_query_limit do |expected| module ExceedQueryLimitHelpers
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
def with_threshold(threshold) def with_threshold(threshold)
@threshold = threshold @threshold = threshold
self self
...@@ -43,7 +30,7 @@ RSpec::Matchers.define :exceed_query_limit do |expected| ...@@ -43,7 +30,7 @@ RSpec::Matchers.define :exceed_query_limit do |expected|
end end
def recorder def recorder
@recorder ||= ActiveRecord::QueryRecorder.new(&@subject_block) @recorder ||= ActiveRecord::QueryRecorder.new(skip_cached: skip_cached, &@subject_block)
end end
def count_queries(queries) def count_queries(queries)
...@@ -61,4 +48,52 @@ RSpec::Matchers.define :exceed_query_limit do |expected| ...@@ -61,4 +48,52 @@ RSpec::Matchers.define :exceed_query_limit do |expected|
@recorder.log_message @recorder.log_message
end end
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 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