Merge branch 'fix-search-issues-n-plus-1' into 'master'

[RUN AS-IF-FOSS] Fix N+1 search scope=issues

See merge request gitlab-org/gitlab!36941

app/controllers/search_controller.rb

@@ -6,7 +6,8 @@ class SearchController < ApplicationController
   include RendersCommits
 
   SCOPE_PRELOAD_METHOD = {
-    projects: :with_web_entity_associations
+    projects: :with_web_entity_associations,
+    issues: :with_web_entity_associations
   }.freeze
 
   around_action :allow_gitaly_ref_name_caching

app/models/issue.rb

@@ -87,6 +87,8 @@ class Issue < ApplicationRecord
   scope :order_created_at_desc, -> { reorder(created_at: :desc) }
 
   scope :preload_associated_models, -> { preload(:assignees, :labels, project: :namespace) }
+  scope :with_web_entity_associations, -> { preload(:author, :project) }
+  scope :with_api_entity_associations, -> { preload(:timelogs, :assignees, :author, :notes, :labels, project: [:route, { namespace: :route }] ) }
   scope :with_label_attributes, ->(label_attributes) { joins(:labels).where(labels: label_attributes) }
   scope :with_alert_management_alerts, -> { joins(:alert_management_alert) }
   scope :with_prometheus_alert_events, -> { joins(:issues_prometheus_alert_events) }

changelogs/unreleased/fix-search-issues-n-plus-1.yml

+---
+title: Avoid N+1 of issue associations in Search
+merge_request: 36941
+author:
+type: performance

ee/spec/requests/search_controller_spec.rb

@@ -15,58 +15,60 @@ RSpec.describe SearchController, type: :request do
     get search_path, params: params
   end
 
-  describe 'GET /search' do
-    context 'when elasticsearch is enabled', :elastic, :sidekiq_inline do
-      before do
-        stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
-      end
+  shared_examples 'an efficient database result' do
+    it 'avoids N+1 database queries' do
+      create(object, *creation_traits, creation_args)
 
-      context 'for merge_request scope' do
-        before do
-          create(:merge_request, target_branch: 'feature_1', source_project: project)
-          create(:merge_request, target_branch: 'feature_2', source_project: project)
-          create(:merge_request, target_branch: 'feature_3', source_project: project)
-          create(:merge_request, target_branch: 'feature_4', source_project: project)
       ensure_elasticsearch_index!
-        end
-
-        it 'avoids N+1 queries' do
-          control = ActiveRecord::QueryRecorder.new(skip_cached: false) { send_search_request(scope: 'merge_requests', search: '*') }
 
-          create(:merge_request, target_branch: 'feature_5', source_project: project)
-          create(:merge_request, target_branch: 'feature_6', source_project: project)
-          create(:merge_request, target_branch: 'feature_7', source_project: project)
-          create(:merge_request, target_branch: 'feature_8', source_project: project)
+      control = ActiveRecord::QueryRecorder.new(skip_cached: false) { send_search_request(params) }
+      create_list(object, 3, *creation_traits, creation_args)
 
       ensure_elasticsearch_index!
 
-          # some N+1 queries still exist
-          expect { send_search_request(scope: 'merge_requests', search: '*') }
-            .not_to exceed_all_query_limit(control)
+      expect { send_search_request(params) }.not_to exceed_all_query_limit(control).with_threshold(threshold)
     end
   end
 
-      context 'for project scope' do
+  describe 'GET /search' do
+    context 'when elasticsearch is enabled', :elastic, :sidekiq_inline do
       before do
-          create(:project, :public)
-          ensure_elasticsearch_index!
+        stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
       end
+      let(:creation_traits) { [] }
 
-        it 'avoids N+1 queries' do
-          control = ActiveRecord::QueryRecorder.new(skip_cached: false) { send_search_request(scope: 'project', search: '*') }
+      context 'for issues scope' do
+        let(:object) { :issue }
+        let(:creation_args) { { project: project } }
+        let(:params) { { search: '*', scope: 'issues' } }
+        let(:threshold) { 0 }
 
-          create_list(:project, 3, :public)
+        it_behaves_like 'an efficient database result'
+      end
 
-          ensure_elasticsearch_index!
+      context 'for merge_request scope' do
+        let(:creation_traits) { [:unique_branches] }
+        let(:object) { :merge_request }
+        let(:creation_args) { { source_project: project } }
+        let(:params) { { search: '*', scope: 'merge_requests' } }
+        let(:threshold) { 0 }
 
+        it_behaves_like 'an efficient database result'
+      end
+
+      context 'for project scope' do
+        let(:creation_traits) { [:public] }
+        let(:object) { :project }
+        let(:creation_args) { {} }
+        let(:params) { { search: '*', scope: 'projects' } }
         # some N+1 queries still exist
         # each project requires 3 extra queries
         #   - one count for forks
         #   - one count for open MRs
         #   - one count for open Issues
-          expect { send_search_request(scope: 'project', search: '*') }
-            .not_to exceed_all_query_limit(control).with_threshold(9)
-        end
+        let(:threshold) { 9 }
+
+        it_behaves_like 'an efficient database result'
       end
     end
   end

spec/requests/search_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe SearchController, type: :request do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, :public, :repository, :wiki_repo, name: 'awesome project', group: group) }
+
+  before_all do
+    login_as(user)
+  end
+
+  def send_search_request(params)
+    get search_path, params: params
+  end
+
+  shared_examples 'an efficient database result' do
+    it 'avoids N+1 database queries' do
+      create(object, *creation_traits, creation_args)
+
+      control = ActiveRecord::QueryRecorder.new(skip_cached: false) { send_search_request(params) }
+      create_list(object, 3, *creation_traits, creation_args)
+
+      expect { send_search_request(params) }.not_to exceed_all_query_limit(control).with_threshold(threshold)
+    end
+  end
+
+  describe 'GET /search' do
+    let(:creation_traits) { [] }
+
+    context 'for issues scope' do
+      let(:object) { :issue }
+      let(:creation_args) { { project: project } }
+      let(:params) { { search: '*', scope: 'issues' } }
+      let(:threshold) { 0 }
+
+      it_behaves_like 'an efficient database result'
+    end
+
+    context 'for merge_request scope' do
+      let(:creation_traits) { [:unique_branches] }
+      let(:object) { :merge_request }
+      let(:creation_args) { { source_project: project } }
+      let(:params) { { search: '*', scope: 'merge_requests' } }
+      let(:threshold) { 0 }
+
+      it_behaves_like 'an efficient database result'
+    end
+
+    context 'for project scope' do
+      let(:creation_traits) { [:public] }
+      let(:object) { :project }
+      let(:creation_args) { {} }
+      let(:params) { { search: '*', scope: 'projects' } }
+      # some N+1 queries still exist
+      # each project requires 3 extra queries
+      #   - one count for forks
+      #   - one count for open MRs
+      #   - one count for open Issues
+      let(:threshold) { 9 }
+
+      it_behaves_like 'an efficient database result'
+    end
+  end
+end