Merge branch 'sh-add-uncached-query-limiter' into 'master'

Remove N+1 query for author in issues API

See merge request gitlab-org/gitlab-ce!19345

changelogs/unreleased/sh-add-uncached-query-limiter.yml

+---
+title: Remove N+1 query for author in issues API
+merge_request:
+author:
+type: performance

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.

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

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

spec/support/helpers/query_recorder.rb

 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]

spec/support/matchers/exceed_query_limit.rb

-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