Merge branch...

Merge branch '221211-improve-performance-of-group-search-api-advanced-merge_requests-scope' into 'master'

Resolve "Improve performance of Group Search API (advanced): merge_requests scope"

Closes #221211

See merge request gitlab-org/gitlab!36072

app/models/merge_request.rb

@@ -265,6 +265,7 @@ class MergeRequest < ApplicationRecord
             *PROJECT_ROUTE_AND_NAMESPACE_ROUTE,
             metrics: [:latest_closed_by, :merged_by])
   }
+
   scope :by_target_branch_wildcard, ->(wildcard_branch_name) do
     where("target_branch LIKE ?", ApplicationRecord.sanitize_sql_like(wildcard_branch_name).tr('*', '%'))
   end

changelogs/unreleased/221211-improve-performance-of-group-search-api-advanced-merge_requests-sc.yml

+---
+title: Improve the search performance for merge requests
+merge_request: 36072
+author:
+type: performance

ee/lib/api/epics.rb

@@ -42,7 +42,7 @@ module API
       get ':id/(-/)epics' do
         epics = paginate(find_epics(finder_params: { group_id: user_group.id })).with_api_entity_associations
 
-        # issuable_metadata is the standard used by the Todo API
+        # issuable_metadata has to be set because `Entities::Epic` doesn't inherit from `Entities::IssuableEntity`
         extra_options = { issuable_metadata: Gitlab::IssuableMetadata.new(current_user, epics).data, with_labels_details: declared_params[:with_labels_details] }
         present epics, epic_options.merge(extra_options)
       end

ee/lib/ee/api/releases.rb

@@ -9,7 +9,7 @@ module EE
         resource :projects, requirements: ::API::API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
           desc 'Create Evidence for a Release' do
             detail 'This feature was introduced in GitLab 12.10.'
-            success Entities::Release
+            success ::API::Entities::Release
           end
           params do
             requires :tag_name, type: String, desc: 'The name of the tag', as: :tag

ee/spec/requests/api/search_spec.rb

@@ -80,8 +80,7 @@ RSpec.describe API::Search do
 
         ensure_elasticsearch_index!
 
-        # Some N+1 queries still exist
-        expect { get api(endpoint, user), params: { scope: 'merge_requests', search: '*' } }.not_to exceed_query_limit(control.count + 16)
+        expect { get api(endpoint, user), params: { scope: 'merge_requests', search: '*' } }.not_to exceed_query_limit(control)
       end
     end
 
@@ -125,7 +124,7 @@ RSpec.describe API::Search do
         ensure_elasticsearch_index!
 
         # Some N+1 queries still exist
-        expect { get api(endpoint, user), params: { scope: 'commits', search: 'folder' } }.not_to exceed_query_limit(control.count + 9)
+        expect { get api(endpoint, user), params: { scope: 'commits', search: 'folder' } }.not_to exceed_query_limit(control).with_threshold(9)
       end
     end
 
@@ -187,8 +186,7 @@ RSpec.describe API::Search do
 
         ensure_elasticsearch_index!
 
-        # Some N+1 queries still exist
-        expect { get api(endpoint, user), params: { scope: 'issues', search: '*' } }.not_to exceed_query_limit(control.count + 2)
+        expect { get api(endpoint, user), params: { scope: 'issues', search: '*' } }.not_to exceed_query_limit(control)
       end
 
       it_behaves_like 'pagination', scope: 'issues'
@@ -213,7 +211,7 @@ RSpec.describe API::Search do
           ensure_elasticsearch_index!
 
           # Some N+1 queries still exist
-          expect { get api(endpoint, user), params: { scope: 'projects', search: '*' } }.not_to exceed_query_limit(control.count + 4)
+          expect { get api(endpoint, user), params: { scope: 'projects', search: '*' } }.not_to exceed_query_limit(control).with_threshold(4)
         end
       end
     end
@@ -234,7 +232,7 @@ RSpec.describe API::Search do
 
         ensure_elasticsearch_index!
 
-        expect { get api(endpoint, user), params: { scope: 'milestones', search: '*' } }.not_to exceed_query_limit(control.count)
+        expect { get api(endpoint, user), params: { scope: 'milestones', search: '*' } }.not_to exceed_query_limit(control)
       end
     end
 

lib/api/entities/issuable_entity.rb

@@ -8,16 +8,39 @@ module API
       expose :title, :description
       expose :state, :created_at, :updated_at
 
-      # Avoids an N+1 query when metadata is included
-      def issuable_metadata(subject, options, method, args = nil)
-        cached_subject = options.dig(:issuable_metadata, subject.id)
-
-        if cached_subject
-          cached_subject[method]
-        else
-          subject.public_send(method, *args) # rubocop: disable GitlabSecurity/PublicSend
+      def presented
+        lazy_issuable_metadata
+
+        super
+      end
+
+      def issuable_metadata
+        options.dig(:issuable_metadata, object.id) || lazy_issuable_metadata
+      end
+
+      protected
+
+      # This method will preload the `issuable_metadata` for the current
+      # entity according to the current top-level entity options, such
+      # as the current_user.
+      def lazy_issuable_metadata
+        BatchLoader.for(object).batch(key: [current_user, :issuable_metadata]) do |models, loader, args|
+          current_user = args[:key].first
+
+          issuable_metadata = Gitlab::IssuableMetadata.new(current_user, models)
+          metadata_by_id = issuable_metadata.data
+
+          models.each do |issuable|
+            loader.call(issuable, metadata_by_id[issuable.id])
+          end
         end
       end
+
+      private
+
+      def current_user
+        options[:current_user]
+      end
     end
   end
 end

lib/api/entities/issue_basic.rb

@@ -21,10 +21,10 @@ module API
         issue.assignees.first
       end
 
-      expose(:user_notes_count)     { |issue, options| issuable_metadata(issue, options, :user_notes_count) }
-      expose(:merge_requests_count) { |issue, options| issuable_metadata(issue, options, :merge_requests_count, options[:current_user]) }
-      expose(:upvotes)              { |issue, options| issuable_metadata(issue, options, :upvotes) }
-      expose(:downvotes)            { |issue, options| issuable_metadata(issue, options, :downvotes) }
+      expose(:user_notes_count)     { |issue, options| issuable_metadata.user_notes_count }
+      expose(:merge_requests_count) { |issue, options| issuable_metadata.merge_requests_count }
+      expose(:upvotes)              { |issue, options| issuable_metadata.upvotes }
+      expose(:downvotes)            { |issue, options| issuable_metadata.downvotes }
       expose :due_date
       expose :confidential
       expose :discussion_locked

lib/api/entities/merge_request_basic.rb

@@ -22,13 +22,11 @@ module API
         MarkupHelper.markdown_field(entity, :description)
       end
       expose :target_branch, :source_branch
-      expose(:user_notes_count) { |merge_request, options| issuable_metadata(merge_request, options, :user_notes_count) }
-      expose(:upvotes)          { |merge_request, options| issuable_metadata(merge_request, options, :upvotes) }
-      expose(:downvotes)        { |merge_request, options| issuable_metadata(merge_request, options, :downvotes) }
-      expose :assignee, using: ::API::Entities::UserBasic do |merge_request|
-        merge_request.assignee
-      end
-      expose :author, :assignees, using: Entities::UserBasic
+      expose(:user_notes_count) { |merge_request, options| issuable_metadata.user_notes_count }
+      expose(:upvotes)          { |merge_request, options| issuable_metadata.upvotes }
+      expose(:downvotes)        { |merge_request, options| issuable_metadata.downvotes }
+
+      expose :author, :assignees, :assignee, using: Entities::UserBasic
       expose :source_project_id, :target_project_id
       expose :labels do |merge_request, options|
         if options[:with_labels_details]
@@ -57,9 +55,12 @@ module API
       expose :discussion_locked
       expose :should_remove_source_branch?, as: :should_remove_source_branch
       expose :force_remove_source_branch?, as: :force_remove_source_branch
-      expose :allow_collaboration, if: -> (merge_request, _) { merge_request.for_fork? }
-      # Deprecated
-      expose :allow_collaboration, as: :allow_maintainer_to_push, if: -> (merge_request, _) { merge_request.for_fork? }
+
+      with_options if: -> (merge_request, _) { merge_request.for_fork? } do
+        expose :allow_collaboration
+        # Deprecated
+        expose :allow_collaboration, as: :allow_maintainer_to_push
+      end
 
       # reference is deprecated in favour of references
       # Introduced [Gitlab 12.6](https://gitlab.com/gitlab-org/gitlab/merge_requests/20354)

lib/api/issues.rb

@@ -107,7 +107,6 @@ module API
           with: Entities::Issue,
           with_labels_details: declared_params[:with_labels_details],
           current_user: current_user,
-          issuable_metadata: Gitlab::IssuableMetadata.new(current_user, issues).data,
           include_subscribed: false
         }
 
@@ -133,7 +132,6 @@ module API
           with: Entities::Issue,
           with_labels_details: declared_params[:with_labels_details],
           current_user: current_user,
-          issuable_metadata: Gitlab::IssuableMetadata.new(current_user, issues).data,
           include_subscribed: false,
           group: user_group
         }
@@ -170,7 +168,6 @@ module API
           with_labels_details: declared_params[:with_labels_details],
           current_user: current_user,
           project: user_project,
-          issuable_metadata: Gitlab::IssuableMetadata.new(current_user, issues).data,
           include_subscribed: false
         }
 

lib/api/merge_requests.rb

@@ -93,7 +93,6 @@ module API
         if params[:view] == 'simple'
           options[:with] = Entities::MergeRequestSimple
         else
-          options[:issuable_metadata] = Gitlab::IssuableMetadata.new(current_user, merge_requests).data
           options[:skip_merge_status_recheck] = !declared_params[:with_merge_status_recheck]
         end
 

lib/gitlab/search_results.rb

@@ -27,6 +27,7 @@ module Gitlab
     end
 
     def objects(scope, page: nil, per_page: DEFAULT_PER_PAGE, without_count: true, preload_method: nil)
+      should_preload = preload_method.present?
       collection = case scope
                    when 'projects'
                      projects
@@ -39,9 +40,11 @@ module Gitlab
                    when 'users'
                      users
                    else
+                     should_preload = false
                      Kaminari.paginate_array([])
                    end
 
+      collection = collection.public_send(preload_method) if should_preload # rubocop:disable GitlabSecurity/PublicSend
       collection = collection.page(page).per(per_page)
 
       without_count ? collection.without_count : collection

spec/lib/api/entities/merge_request_basic_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::API::Entities::MergeRequestBasic do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :public) }
+  let_it_be(:merge_request) { create(:merge_request) }
+  let_it_be(:labels) { create_list(:label, 3) }
+  let_it_be(:merge_requests) { create_list(:labeled_merge_request, 10, :unique_branches, :with_diffs, labels: labels) }
+
+  # This mimics the behavior of the `Grape::Entity` serializer
+  def present(obj)
+    described_class.new(obj).presented
+  end
+
+  context "with :with_api_entity_associations scope" do
+    let(:scope) { MergeRequest.with_api_entity_associations }
+
+    it "avoids N+1 queries" do
+      query = scope.find(merge_request.id)
+
+      control = ActiveRecord::QueryRecorder.new do
+        present(query).to_json
+      end
+
+      # stub the `head_commit_sha` as it will trigger a
+      # backward compatibility query that is out-of-scope
+      # for this test whenever it is `nil`
+      allow_any_instance_of(MergeRequestDiff).to receive(:head_commit_sha).and_return(Gitlab::Git::BLANK_SHA)
+
+      query = scope.all
+      batch = ActiveRecord::QueryRecorder.new do
+        entities = query.map(&method(:present))
+
+        entities.to_json
+      end
+
+      # The current threshold is 3 query per entity maximum.
+      expect(batch.count).to be_within(3 * query.count).of(control.count)
+    end
+  end
+end