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`.
This commit is contained in:
parent
bb2a847c83
commit
c26cbfcdfb
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
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
|
|||
|
||||
> **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.
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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]
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue