Revert "Avoid loading objects from DB in ES results"

This reverts commit d9cb907c.

app/controllers/search_controller.rb

@@ -25,7 +25,6 @@ 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,8 +72,6 @@ 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,10 +52,6 @@ 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 }.merge(@display_options)
+            = render 'shared/projects/list', projects: @search_objects, pipeline_status: false
         - 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 namespace_project_issue_path(issue.project.namespace.becomes(Namespace), issue.project, issue) do
+    = link_to [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 namespace_project_merge_request_path(merge_request.target_project.namespace.becomes(Namespace), merge_request.target_project, merge_request) do
+    = link_to [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 namespace_project_milestone_path(milestone.project.namespace.becomes(Namespace), milestone.project, milestone) do
+    = link_to [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, page: nil, per_page: nil)
+      def basic_query_hash(fields, query)
         query_hash = if query.present?
                        {
                          query: {
@@ -360,9 +360,6 @@ 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, page: options[:page], per_page: options[:per_page])
+            basic_query_hash(%w(title^2 description), query)
           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, page: options[:page], per_page: options[:per_page])
+            basic_query_hash(%w(title^2 description), query)
           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, :iid, :title, :description, :project_id, :created_at, :updated_at].each do |attr|
+        [:id, :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, page: options[:page], per_page: options[:per_page])
+        query_hash = basic_query_hash(options[:in], query)
 
         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, page: options[:page], per_page: options[:per_page])
+        query_hash = basic_query_hash(options[:in], query)
 
         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,8 +5,6 @@ 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
@@ -27,13 +25,13 @@ module Gitlab
       def objects(scope, page = nil)
         case scope
         when 'projects'
-          projects(page: page, per_page: per_page)
+          projects.page(page).per(per_page).records
         when 'issues'
-          issues(page: page, per_page: per_page)
+          issues.page(page).per(per_page).records
         when 'merge_requests'
-          merge_requests(page: page, per_page: per_page)
+          merge_requests.page(page).per(per_page).records
         when 'milestones'
-          milestones(page: page, per_page: per_page)
+          milestones.page(page).per(per_page).records
         when 'blobs'
           blobs.page(page).per(per_page)
         when 'wiki_blobs'
@@ -47,17 +45,6 @@ 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
@@ -156,40 +143,19 @@ module Gitlab
         }
       end
 
-      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)
+      def projects
         strong_memoize(:projects) do
-          search(Project, query, base_options, page: page, per_page: per_page)
+          Project.elastic_search(query, options: base_options)
         end
       end
 
-      # See the comment for #commits for more info on why we memoize this way
-      def issues(page: 1, per_page: 20)
+      def issues
         strong_memoize(:issues) do
-          search(Issue, query, base_options, page: page, per_page: per_page)
+          Issue.elastic_search(query, options: base_options)
         end
       end
 
-      # See the comment for #commits for more info on why we memoize this way
-      def milestones(page: 1, per_page: 20)
+      def milestones
         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
@@ -198,18 +164,17 @@ module Gitlab
           options = base_options
           options[:features] = [:issues, :merge_requests]
 
-          search(Milestone, query, options, page: page, per_page: per_page)
+          Milestone.elastic_search(query, options: options)
         end
       end
 
-      # See the comment for #commits for more info on why we memoize this way
-      def merge_requests(page: 1, per_page: 20)
+      def merge_requests
         strong_memoize(:merge_requests) do
-          search(MergeRequest, query, base_options.merge(project_ids: non_guest_project_ids), page: page, per_page: per_page)
+          options = base_options.merge(project_ids: non_guest_project_ids)
+          MergeRequest.elastic_search(query, options: options)
         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?
 
@@ -226,7 +191,6 @@ 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', :elastic do
+describe 'Global elastic search' 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').map(&:id)
+      issues = results.objects('issues')
 
-      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(issues).to include issue
+      expect(issues).not_to include security_issue_1
+      expect(issues).not_to include security_issue_2
       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').map(&:id)
+      issues = results.objects('issues')
 
-      expect(issues).to include issue.id
-      expect(issues).to include security_issue_1.id
-      expect(issues).not_to include security_issue_2.id
+      expect(issues).to include issue
+      expect(issues).to include security_issue_1
+      expect(issues).not_to include security_issue_2
       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').map(&:id)
+      issues = results.objects('issues')
 
-      expect(issues).to include issue.id
-      expect(issues).not_to include security_issue_1.id
-      expect(issues).to include security_issue_2.id
+      expect(issues).to include issue
+      expect(issues).not_to include security_issue_1
+      expect(issues).to include security_issue_2
       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').map(&:id)
+      issues = results.objects('issues')
 
-      expect(issues).to include issue.id
-      expect(issues).to include security_issue_1.id
-      expect(issues).to include security_issue_2.id
+      expect(issues).to include issue
+      expect(issues).to include security_issue_1
+      expect(issues).to include security_issue_2
       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').map(&:id)
+      issues = results.objects('issues')
 
-      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(issues).to include issue
+      expect(issues).not_to include security_issue_1
+      expect(issues).not_to include security_issue_2
       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').map(&:id)
+      issues = results.objects('issues')
 
-      expect(issues).to include issue.id
-      expect(issues).to include security_issue_1.id
-      expect(issues).to include security_issue_2.id
+      expect(issues).to include issue
+      expect(issues).to include security_issue_1
+      expect(issues).to include security_issue_2
       expect(results.issues_count).to eq 3
     end
   end

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

@@ -39,7 +39,6 @@ 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').map(&:id) }
+      subject { results.objects('projects') }
 
       context 'in parent group' do
         let(:search_group) { nested_group.parent }
 
-        it { is_expected.to match_array([project1.id, project2.id, project3.id]) }
+        it { is_expected.to match_array([project1, project2, project3]) }
       end
 
       context 'in subgroup' do
         let(:search_group) { nested_group }
 
-        it { is_expected.to match_array([project1.id, project2.id]) }
+        it { is_expected.to match_array([project1, project2]) }
       end
     end
   end

lib/gitlab/search_results.rb

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