Merge branch '10854-elasticsearch_avoid_db' into 'master'

Avoid loading database objects in ES result

Closes #10854

See merge request gitlab-org/gitlab-ee!12691

app/controllers/search_controller.rb

@@ -25,6 +25,7 @@ class SearchController < ApplicationController
     @show_snippets = search_service.show_snippets?
     @search_results = search_service.search_results
     @search_objects = search_service.search_objects
+    @display_options = search_service.display_options
 
     render_commits if @scope == 'commits'
     eager_load_user_status if @scope == 'users'

app/models/project_feature.rb

@@ -72,6 +72,8 @@ class ProjectFeature < ApplicationRecord
   default_value_for :wiki_access_level,           value: ENABLED, allows_nil: false
   default_value_for :repository_access_level,     value: ENABLED, allows_nil: false
 
+  scope :for_project_id, -> (project) { where(project: project) }
+
   def feature_available?(feature, user)
     # This feature might not be behind a feature flag at all, so default to true
     return false unless ::Feature.enabled?(feature, user, default_enabled: true)

app/services/search_service.rb

@@ -52,6 +52,10 @@ class SearchService
     @search_objects ||= search_results.objects(scope, params[:page])
   end
 
+  def display_options
+    @display_options ||= search_results.display_options(scope)
+  end
+
   private
 
   def search_service

app/views/search/_results.html.haml

@@ -21,7 +21,7 @@
       .search-results
         - if @scope == 'projects'
           .term
-            = render 'shared/projects/list', projects: @search_objects, pipeline_status: false
+            = render 'shared/projects/list', { projects: @search_objects, pipeline_status: false }.merge(@display_options)
         - else
           - locals = { projects: blob_projects(@search_objects) } if %w[blobs wiki_blobs].include?(@scope)
           = render partial: "search/results/#{@scope.singularize}", collection: @search_objects, locals: locals

app/views/search/results/_issue.html.haml

 .search-result-row
   %h4
     = confidential_icon(issue)
-    = link_to [issue.project.namespace.becomes(Namespace), issue.project, issue] do
+    = link_to namespace_project_issue_path(issue.project.namespace.becomes(Namespace), issue.project, issue) do
       %span.term.str-truncated= issue.title
     - if issue.closed?
       %span.badge.badge-danger.prepend-left-5= _("Closed")

app/views/search/results/_merge_request.html.haml

 .search-result-row
   %h4
-    = link_to [merge_request.target_project.namespace.becomes(Namespace), merge_request.target_project, merge_request] do
+    = link_to namespace_project_merge_request_path(merge_request.target_project.namespace.becomes(Namespace), merge_request.target_project, merge_request) do
       %span.term.str-truncated= merge_request.title
     - if merge_request.merged?
       %span.badge.badge-primary.prepend-left-5= _("Merged")

app/views/search/results/_milestone.html.haml

 .search-result-row
   %h4
-    = link_to [milestone.project.namespace.becomes(Namespace), milestone.project, milestone] do
+    = link_to namespace_project_milestone_path(milestone.project.namespace.becomes(Namespace), milestone.project, milestone) do
       %span.term.str-truncated= milestone.title
 
   - if milestone.description.present?

ee/app/models/concerns/elastic/application_search.rb

@@ -331,7 +331,7 @@ module Elastic
         self.import(options)
       end
 
-      def basic_query_hash(fields, query)
+      def basic_query_hash(fields, query, page: nil, per_page: nil)
         query_hash = if query.present?
                        {
                          query: {
@@ -360,6 +360,9 @@ module Elastic
                        }
                      end
 
+        query_hash[:size] = per_page if per_page
+        query_hash[:from] = per_page * (page - 1) if per_page && page
+
         query_hash[:sort] = [
           { updated_at: { order: :desc } },
           :_score

ee/app/models/concerns/elastic/issues_search.rb

@@ -30,7 +30,7 @@ module Elastic
           if query =~ /#(\d+)\z/
             iid_query_hash(Regexp.last_match(1))
           else
-            basic_query_hash(%w(title^2 description), query)
+            basic_query_hash(%w(title^2 description), query, page: options[:page], per_page: options[:per_page])
           end
 
         options[:features] = 'issues'

ee/app/models/concerns/elastic/merge_requests_search.rb

@@ -46,7 +46,7 @@ module Elastic
           if query =~ /\!(\d+)\z/
             iid_query_hash(Regexp.last_match(1))
           else
-            basic_query_hash(%w(title^2 description), query)
+            basic_query_hash(%w(title^2 description), query, page: options[:page], per_page: options[:per_page])
           end
 
         options[:features] = 'merge_requests'

ee/app/models/concerns/elastic/milestones_search.rb

@@ -12,7 +12,7 @@ module Elastic
         # https://gitlab.com/gitlab-org/gitlab-ee/issues/349
         data = {}
 
-        [:id, :title, :description, :project_id, :created_at, :updated_at].each do |attr|
+        [:id, :iid, :title, :description, :project_id, :created_at, :updated_at].each do |attr|
           data[attr.to_s] = safely_read_attribute_for_elasticsearch(attr)
         end
 
@@ -26,7 +26,7 @@ module Elastic
       def self.elastic_search(query, options: {})
         options[:in] = %w(title^2 description)
 
-        query_hash = basic_query_hash(options[:in], query)
+        query_hash = basic_query_hash(options[:in], query, page: options[:page], per_page: options[:per_page])
 
         query_hash = project_ids_filter(query_hash, options)
 

ee/app/models/concerns/elastic/projects_search.rb

@@ -65,7 +65,7 @@ module Elastic
       def self.elastic_search(query, options: {})
         options[:in] = %w(name^10 name_with_namespace^2 path_with_namespace path^9 description)
 
-        query_hash = basic_query_hash(options[:in], query)
+        query_hash = basic_query_hash(options[:in], query, page: options[:page], per_page: options[:per_page])
 
         filters = []
 

ee/changelogs/unreleased/10854-elasticsearch_avoid_db.yml

+---
+title: Avoid loading database objects for Elasticsearch results
+merge_request: 12691
+author:
+type: performance

ee/lib/elasticsearch/lite_project.rb

+# frozen_string_literal: true
+
+module Elasticsearch
+  class LiteProject
+    include Gitlab::Utils::StrongMemoize
+
+    attr_accessor :id, :name, :path, :description, :namespace_id, :create_at, :updated_at, :archived,
+                  :visibility_level, :last_activity_at, :name_with_namespace, :path_with_namespace,
+                  :issues_access_level, :merge_requests_access_level, :snippets_access_level, :wiki_access_level,
+                  :repository_access_level
+    attr_accessor :pipeline_status, :commit, :creator
+
+    alias_attribute :last_activity_date, :last_activity_at
+
+    def initialize(raw_project_hash)
+      raw_project_hash.each do |key, value|
+        value = value.to_datetime if key =~ /_at$/
+        self.instance_variable_set(:"@#{key}", value)
+      end
+    end
+
+    # only used for routing and results display, so we trick Rails here
+    def namespace
+      # can't use an OpenStruct because to_param is a defined method on it
+      Struct.new(:to_param, :human_name).new(
+        path_with_namespace.sub(/\/#{path}$/, ''),
+        name_with_namespace.sub(/\s+\/\s+#{name}$/, '')
+      )
+    end
+
+    def route
+      # Creates an object that has a `cache_key` attribute set to nil
+      Struct.new(:cache_key).new(nil)
+    end
+
+    def pending_delete?
+      false
+    end
+
+    def cache_key
+      "lite_projects/#{id}-#{updated_at.utc.to_s(:number)}"
+    end
+
+    def to_param
+      path&.to_s
+    end
+
+    def model_name
+      OpenStruct.new(param_key: 'project')
+    end
+
+    def banzai_render_context(field)
+      return unless field == :description
+
+      { pipeline: :description, project: self }
+    end
+
+    def default_issues_tracker?
+      true
+    end
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    # This method is required by the OpenMergeRequestsCountService
+    def merge_requests
+      strong_memoize(:merge_requests) do
+        MergeRequest.opened.where(project_id: self.id)
+      end
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+
+    def forks_count
+      Projects::ForksCountService.new(self).count
+    end
+
+    def open_issues_count(current_user = nil)
+      Projects::OpenIssuesCountService.new(self, current_user).count
+    end
+
+    def open_merge_requests_count
+      Projects::OpenMergeRequestsCountService.new(self).count
+    end
+  end
+end

ee/lib/elasticsearch/result_objects.rb

+# frozen_string_literal: true
+
+module Elasticsearch
+  module ResultObjects
+    # Here we refine our models to learn how to load from Elasticsearch without hitting the database
+    # In order to use these methods in another class you first have to `using Elasticsearch::ResultObjects`
+    # and you can then use the model objects directly
+
+    ES_ATTRIBUTES = %i(join_field type).freeze
+
+    refine ::Project.singleton_class do
+      def load_from_elasticsearch(es_response, current_user: nil)
+        es_response.results.response.map(&:_source).then do |projects|
+          projects.map do |project|
+            ::Elasticsearch::LiteProject.new(project)
+          end
+        end
+      end
+    end
+
+    refine ::Issue.singleton_class do
+      def load_from_elasticsearch(es_response, current_user:)
+        es_response.results.response.map(&:_source).then do |issues|
+          projects = ::ProjectsFinder.new(
+            current_user: current_user,
+            project_ids_relation: issues.map(&:project_id)
+          ).execute.index_by(&:id)
+
+          issues.map do |issue|
+            issue[:project] = projects[issue.delete(:project_id)]
+            issue[:assignee_ids] = issue.delete(:assignee_id)
+            issue.except!(*ES_ATTRIBUTES)
+
+            new(issue)
+          end
+        end
+      end
+    end
+
+    refine ::MergeRequest.singleton_class do
+      def load_from_elasticsearch(es_response, current_user:)
+        es_response.results.response.map(&:_source).then do |merge_requests|
+          # rubocop: disable CodeReuse/ActiveRecord
+          projects = ::ProjectsFinder.new(
+            current_user: current_user,
+            project_ids_relation: merge_requests.map(&:target_project_id)
+          ).execute.includes(:route, namespace: [:route]).index_by(&:id)
+          # rubocop: enable CodeReuse/ActiveRecord
+
+          merge_requests.map do |merge_request|
+            merge_request[:target_project] = projects[merge_request.delete(:target_project_id)]
+            merge_request.except!(*ES_ATTRIBUTES)
+
+            new(merge_request)
+          end
+        end
+      end
+    end
+
+    refine ::Milestone.singleton_class do
+      def load_from_elasticsearch(es_response, current_user:)
+        es_response.results.response.map(&:_source).then do |milestones|
+          projects = ::ProjectsFinder.new(
+            current_user: current_user,
+            project_ids_relation: milestones.map(&:project_id)
+          ).execute.index_by(&:id)
+
+          milestones.map do |milestone|
+            milestone[:project] = projects[milestone.delete(:project_id)]
+            milestone.except!(*ES_ATTRIBUTES)
+
+            new(milestone)
+          end
+        end
+      end
+    end
+  end
+end

ee/lib/gitlab/elastic/search_results.rb

@@ -5,6 +5,8 @@ module Gitlab
     class SearchResults
       include Gitlab::Utils::StrongMemoize
 
+      using Elasticsearch::ResultObjects
+
       attr_reader :current_user, :query, :public_and_internal_projects
 
       # Limit search results by passed project ids
@@ -25,13 +27,13 @@ module Gitlab
       def objects(scope, page = nil)
         case scope
         when 'projects'
-          projects.page(page).per(per_page).records
+          projects(page: page, per_page: per_page)
         when 'issues'
-          issues.page(page).per(per_page).records
+          issues(page: page, per_page: per_page)
         when 'merge_requests'
-          merge_requests.page(page).per(per_page).records
+          merge_requests(page: page, per_page: per_page)
         when 'milestones'
-          milestones.page(page).per(per_page).records
+          milestones(page: page, per_page: per_page)
         when 'blobs'
           blobs.page(page).per(per_page)
         when 'wiki_blobs'
@@ -45,6 +47,17 @@ module Gitlab
         end
       end
 
+      def display_options(scope)
+        case scope
+        when 'projects'
+          {
+            stars: false
+          }
+        else
+          {}
+        end
+      end
+
       def generic_search_results
         @generic_search_results ||= Gitlab::SearchResults.new(current_user, limit_projects, query)
       end
@@ -143,19 +156,40 @@ module Gitlab
         }
       end
 
-      def projects
+      def paginate_array(collection, total_count, page, per_page)
+        offset = per_page * (page - 1)
+        Kaminari.paginate_array(collection, total_count: total_count, limit: per_page, offset: offset)
+      end
+
+      def search(model, query, options, page: 1, per_page: 20)
+        page = (page || 1).to_i
+
+        response = model.elastic_search(
+          query,
+          options: options.merge(page: page, per_page: per_page)
+        )
+
+        results = model.load_from_elasticsearch(response, current_user: current_user)
+
+        paginate_array(results, response.total_count, page, per_page)
+      end
+
+      # See the comment for #commits for more info on why we memoize this way
+      def projects(page: 1, per_page: 20)
         strong_memoize(:projects) do
-          Project.elastic_search(query, options: base_options)
+          search(Project, query, base_options, page: page, per_page: per_page)
         end
       end
 
-      def issues
+      # See the comment for #commits for more info on why we memoize this way
+      def issues(page: 1, per_page: 20)
         strong_memoize(:issues) do
-          Issue.elastic_search(query, options: base_options)
+          search(Issue, query, base_options, page: page, per_page: per_page)
         end
       end
 
-      def milestones
+      # See the comment for #commits for more info on why we memoize this way
+      def milestones(page: 1, per_page: 20)
         strong_memoize(:milestones) do
           # Must pass 'issues' and 'merge_requests' to check
           # if any of the features is available for projects in Elastic::ApplicationSearch#project_ids_query
@@ -164,17 +198,18 @@ module Gitlab
           options = base_options
           options[:features] = [:issues, :merge_requests]
 
-          Milestone.elastic_search(query, options: options)
+          search(Milestone, query, options, page: page, per_page: per_page)
         end
       end
 
-      def merge_requests
+      # See the comment for #commits for more info on why we memoize this way
+      def merge_requests(page: 1, per_page: 20)
         strong_memoize(:merge_requests) do
-          options = base_options.merge(project_ids: non_guest_project_ids)
-          MergeRequest.elastic_search(query, options: options)
+          search(MergeRequest, query, base_options.merge(project_ids: non_guest_project_ids), page: page, per_page: per_page)
         end
       end
 
+      # See the comment for #commits for more info on why we memoize this way
       def blobs
         return Kaminari.paginate_array([]) if query.blank?
 
@@ -191,6 +226,7 @@ module Gitlab
         end
       end
 
+      # See the comment for #commits for more info on why we memoize this way
       def wiki_blobs
         return Kaminari.paginate_array([]) if query.blank?
 

ee/spec/features/search/elastic/global_search_spec.rb

 require 'spec_helper'
 
-describe 'Global elastic search' do
+describe 'Global elastic search', :elastic do
   let(:user) { create(:user) }
   let(:project) { create(:project, :repository, :wiki_repo, namespace: user.namespace) }
 
   before do
     stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
-    Gitlab::Elastic::Helper.create_empty_index
 
     project.add_maintainer(user)
     sign_in(user)
   end
 
   after do
-    Gitlab::Elastic::Helper.delete_index
     stub_ee_application_setting(elasticsearch_search: false, elasticsearch_indexing: false)
   end
 
+  shared_examples 'a pure Elasticsearch result' do
+    it 'avoids N+1 database queries' do
+      create(object, creation_args)
+      Gitlab::Elastic::Helper.refresh_index
+
+      control_count = ActiveRecord::QueryRecorder.new { visit path }.count
+
+      create_list(object, 10, creation_args)
+      Gitlab::Elastic::Helper.refresh_index
+
+      control_count = control_count + (10 * query_count_multiplier) + 1
+
+      expect { visit path }.not_to exceed_query_limit(control_count)
+    end
+  end
+
+  describe 'I do not overload the database' do
+    context 'searching issues' do
+      let(:object) { :issue }
+      let(:creation_args) { { project: project, title: 'initial' } }
+      let(:path) { search_path(search: 'initial', scope: 'issues') }
+      let(:query_count_multiplier) { 0 }
+
+      it_behaves_like 'a pure Elasticsearch result'
+    end
+
+    context 'searching projects' do
+      let(:object) { :project }
+      let(:creation_args) { { namespace: user.namespace } }
+      let(:path) { search_path(search: 'project*', scope: 'projects') }
+      # Each Project requires 4 extra queries: one for each "count" (forks, open MRs, open Issues) and one for access level
+      let(:query_count_multiplier) { 4 }
+
+      it_behaves_like 'a pure Elasticsearch result'
+    end
+
+    context 'searching merge requests' do
+      it 'avoids N+1 database queries' do
+        path = search_path(search: 'initial', scope: 'merge_requests')
+
+        create(:merge_request, title: 'initial', source_project: project)
+        Gitlab::Elastic::Helper.refresh_index
+
+        control_count = ActiveRecord::QueryRecorder.new { visit path }.count
+
+        merge_requests = create_list(:merge_request, 10, title: 'initial')
+        merge_requests.each { |mr| mr.target_project.add_maintainer(user) }
+        Gitlab::Elastic::Helper.refresh_index
+
+        # Each MR loaded has a Route load that has been tricky to track down
+        expect { visit path }.not_to exceed_query_limit(control_count + 11)
+      end
+    end
+
+    context 'searching milestones' do
+      let(:object) { :milestone }
+      let(:creation_args) { { project: project } }
+      let(:path) { search_path(search: 'milestone*', scope: 'milestones') }
+      let(:query_count_multiplier) { 0 }
+
+      it_behaves_like 'a pure Elasticsearch result'
+    end
+  end
+
   describe 'I search through the issues and I see pagination' do
     before do
       create_list(:issue, 21, project: project, title: 'initial')

ee/spec/lib/gitlab/elastic/project_search_results_spec.rb

@@ -162,31 +162,31 @@ describe Gitlab::Elastic::ProjectSearchResults do
 
     it 'does not list project confidential issues for non project members' do
       results = described_class.new(non_member, query, project.id)
-      issues = results.objects('issues')
+      issues = results.objects('issues').map(&:id)
 
-      expect(issues).to include issue
-      expect(issues).not_to include security_issue_1
-      expect(issues).not_to include security_issue_2
+      expect(issues).to include issue.id
+      expect(issues).not_to include security_issue_1.id
+      expect(issues).not_to include security_issue_2.id
       expect(results.issues_count).to eq 1
     end
 
     it 'lists project confidential issues for author' do
       results = described_class.new(author, query, project.id)
-      issues = results.objects('issues')
+      issues = results.objects('issues').map(&:id)
 
-      expect(issues).to include issue
-      expect(issues).to include security_issue_1
-      expect(issues).not_to include security_issue_2
+      expect(issues).to include issue.id
+      expect(issues).to include security_issue_1.id
+      expect(issues).not_to include security_issue_2.id
       expect(results.issues_count).to eq 2
     end
 
     it 'lists project confidential issues for assignee' do
       results = described_class.new(assignee, query, project.id)
-      issues = results.objects('issues')
+      issues = results.objects('issues').map(&:id)
 
-      expect(issues).to include issue
-      expect(issues).not_to include security_issue_1
-      expect(issues).to include security_issue_2
+      expect(issues).to include issue.id
+      expect(issues).not_to include security_issue_1.id
+      expect(issues).to include security_issue_2.id
       expect(results.issues_count).to eq 2
     end
 
@@ -194,11 +194,11 @@ describe Gitlab::Elastic::ProjectSearchResults do
       project.add_developer(member)
 
       results = described_class.new(member, query, project.id)
-      issues = results.objects('issues')
+      issues = results.objects('issues').map(&:id)
 
-      expect(issues).to include issue
-      expect(issues).to include security_issue_1
-      expect(issues).to include security_issue_2
+      expect(issues).to include issue.id
+      expect(issues).to include security_issue_1.id
+      expect(issues).to include security_issue_2.id
       expect(results.issues_count).to eq 3
     end
 
@@ -206,21 +206,21 @@ describe Gitlab::Elastic::ProjectSearchResults do
       project.add_guest(member)
 
       results = described_class.new(member, query, project.id)
-      issues = results.objects('issues')
+      issues = results.objects('issues').map(&:id)
 
-      expect(issues).to include issue
-      expect(issues).not_to include security_issue_1
-      expect(issues).not_to include security_issue_2
+      expect(issues).to include issue.id
+      expect(issues).not_to include security_issue_1.id
+      expect(issues).not_to include security_issue_2.id
       expect(results.issues_count).to eq 1
     end
 
     it 'lists all project issues for admin' do
       results = described_class.new(admin, query, project.id)
-      issues = results.objects('issues')
+      issues = results.objects('issues').map(&:id)
 
-      expect(issues).to include issue
-      expect(issues).to include security_issue_1
-      expect(issues).to include security_issue_2
+      expect(issues).to include issue.id
+      expect(issues).to include security_issue_1.id
+      expect(issues).to include security_issue_2.id
       expect(results.issues_count).to eq 3
     end
   end

ee/spec/models/concerns/elastic/milestone_spec.rb

@@ -39,6 +39,7 @@ describe Milestone, :elastic do
 
     expected_hash = milestone.attributes.extract!(
       'id',
+      'iid',
       'title',
       'description',
       'project_id',

ee/spec/services/search/group_service_spec.rb

@@ -40,18 +40,18 @@ describe Search::GroupService, :elastic do
     end
 
     context 'finding projects by name' do
-      subject { results.objects('projects') }
+      subject { results.objects('projects').map(&:id) }
 
       context 'in parent group' do
         let(:search_group) { nested_group.parent }
 
-        it { is_expected.to match_array([project1, project2, project3]) }
+        it { is_expected.to match_array([project1.id, project2.id, project3.id]) }
       end
 
       context 'in subgroup' do
         let(:search_group) { nested_group }
 
-        it { is_expected.to match_array([project1, project2]) }
+        it { is_expected.to match_array([project1.id, project2.id]) }
       end
     end
   end

lib/gitlab/search_results.rb

@@ -85,6 +85,10 @@ module Gitlab
       UsersFinder.new(current_user, search: query).execute
     end
 
+    def display_options(_scope)
+      {}
+    end
+
     private
 
     def projects