Merge branch '12091-remove-elasticsearch-lite-project' into 'master'

Partially revert d9cb907c: "Avoid loading objects from DB in ES results"

Closes #12091

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

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

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

@@ -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/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)
+          eager_load(projects, page, eager: [:route, :namespace])
         when 'issues'
-          issues(page: page, per_page: per_page)
+          eager_load(issues, page, eager: { project: [:route, :namespace] })
         when 'merge_requests'
-          merge_requests(page: page, per_page: per_page)
+          eager_load(merge_requests, page, eager: { target_project: [:route, :namespace] })
         when 'milestones'
-          milestones(page: page, per_page: per_page)
+          eager_load(milestones, page, eager: { project: [:route, :namespace] })
         when 'blobs'
           blobs.page(page).per(per_page)
         when 'wiki_blobs'
@@ -47,15 +45,19 @@ module Gitlab
         end
       end
 
-      def display_options(scope)
-        case scope
-        when 'projects'
-          {
-            stars: false
-          }
-        else
-          {}
-        end
+      # Apply some eager loading to the `records` of an ES result object without
+      # losing pagination information
+      def eager_load(es_result, page, eager:)
+        page ||= 1
+        paginated_base = es_result.page(page).per(per_page)
+        relation = paginated_base.records.includes(eager) # rubocop:disable CodeReuse/ActiveRecord
+
+        Kaminari.paginate_array(
+          relation,
+          total_count: paginated_base.total_count,
+          limit: per_page,
+          offset: per_page * (page - 1)
+        )
       end
 
       def generic_search_results
@@ -156,40 +158,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 +179,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 +206,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

@@ -15,7 +15,7 @@ describe 'Global elastic search', :elastic do
     stub_ee_application_setting(elasticsearch_search: false, elasticsearch_indexing: false)
   end
 
-  shared_examples 'a pure Elasticsearch result' do
+  shared_examples 'an efficient database result' do
     it 'avoids N+1 database queries' do
       create(object, creation_args)
       Gitlab::Elastic::Helper.refresh_index
@@ -38,7 +38,7 @@ describe 'Global elastic search', :elastic do
       let(:path) { search_path(search: 'initial', scope: 'issues') }
       let(:query_count_multiplier) { 0 }
 
-      it_behaves_like 'a pure Elasticsearch result'
+      it_behaves_like 'an efficient database result'
     end
 
     context 'searching projects' do
@@ -48,25 +48,16 @@ describe 'Global elastic search', :elastic do
       # 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'
+      it_behaves_like 'an efficient database 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
+      let(:object) { :merge_request }
+      let(:creation_args) { { title: 'initial' } }
+      let(:path) { search_path(search: 'initial*', scope: 'merge_requests') }
+      let(:query_count_multiplier) { 0 }
 
-        # 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
+      it_behaves_like 'an efficient database result'
     end
 
     context 'searching milestones' do
@@ -75,7 +66,7 @@ describe 'Global elastic search', :elastic do
       let(:path) { search_path(search: 'milestone*', scope: 'milestones') }
       let(:query_count_multiplier) { 0 }
 
-      it_behaves_like 'a pure Elasticsearch result'
+      it_behaves_like 'an efficient database result'
     end
   end
 

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/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