Merge branch 'es_avoid_double_calls' into 'master'

Avoid calling Elasticsearch results twice

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

ee/changelogs/unreleased/es_avoid_double_calls.yml

+---
+title: Avoid hitting Elasticsearch more than once on search
+merge_request: 13120
+author:
+type: performance

ee/lib/gitlab/elastic/search_results.rb

@@ -3,6 +3,8 @@
 module Gitlab
   module Elastic
     class SearchResults
+      include Gitlab::Utils::StrongMemoize
+
       attr_reader :current_user, :query, :public_and_internal_projects
 
       # Limit search results by passed project ids
@@ -142,33 +144,41 @@ module Gitlab
       end
 
       def projects
-        Project.elastic_search(query, options: base_options)
+        strong_memoize(:projects) do
+          Project.elastic_search(query, options: base_options)
+        end
       end
 
       def issues
-        Issue.elastic_search(query, options: base_options)
+        strong_memoize(:issues) do
+          Issue.elastic_search(query, options: base_options)
+        end
       end
 
       def milestones
-        # Must pass 'issues' and 'merge_requests' to check
-        # if any of the features is available for projects in Elastic::ApplicationSearch#project_ids_query
-        # Otherwise it will ignore project_ids and return milestones
-        # from projects with milestones disabled.
-        options = base_options
-        options[:features] = [:issues, :merge_requests]
-
-        Milestone.elastic_search(query, options: options)
+        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
+          # Otherwise it will ignore project_ids and return milestones
+          # from projects with milestones disabled.
+          options = base_options
+          options[:features] = [:issues, :merge_requests]
+
+          Milestone.elastic_search(query, options: options)
+        end
       end
 
       def merge_requests
-        options = base_options.merge(project_ids: non_guest_project_ids)
-        MergeRequest.elastic_search(query, options: options)
+        strong_memoize(:merge_requests) do
+          options = base_options.merge(project_ids: non_guest_project_ids)
+          MergeRequest.elastic_search(query, options: options)
+        end
       end
 
       def blobs
-        if query.blank?
-          Kaminari.paginate_array([])
-        else
+        return Kaminari.paginate_array([]) if query.blank?
+
+        strong_memoize(:blobs) do
           opt = {
             additional_filter: repository_filter
           }
@@ -182,9 +192,9 @@ module Gitlab
       end
 
       def wiki_blobs
-        if query.blank?
-          Kaminari.painate_array([])
-        else
+        return Kaminari.paginate_array([]) if query.blank?
+
+        strong_memoize(:wiki_blobs) do
           opt = {
             additional_filter: wiki_filter
           }
@@ -197,10 +207,16 @@ module Gitlab
         end
       end
 
+      # We're only memoizing once because this object only ever gets used to show a single page of results
+      # during its lifetime. We _must_ memoize the page we want because `#commits_count` does not have any
+      # inkling of the current page we're on - if we were to memoize with dynamic parameters we would end up
+      # hitting ES twice for any page that's not page 1, and that's something we want to avoid.
+      #
+      # It is safe to memoize the page we get here because this method is _always_ called before `#commits_count`
       def commits(page: 1, per_page: 20)
-        if query.blank?
-          Kaminari.paginate_array([])
-        else
+        return Kaminari.paginate_array([]) if query.blank?
+
+        strong_memoize(:commits) do
           options = {
             additional_filter: repository_filter
           }

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

@@ -119,5 +119,21 @@ describe 'Global elastic search' do
       expect(page).to have_selector('.commit-row-description')
       expect(page).to have_selector('.project-namespace')
     end
+
+    it 'shows proper page 2 results' do
+      visit dashboard_projects_path
+
+      fill_in "search", with: "add"
+      click_button "Go"
+
+      expected_message = "Add directory structure for tree_helper spec"
+
+      select_filter("Commits")
+      expect(page).not_to have_content(expected_message)
+
+      click_link 'Next'
+
+      expect(page).to have_content(expected_message)
+    end
   end
 end

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

 # coding: utf-8
 require 'spec_helper'
 
-describe Gitlab::Elastic::SearchResults do
+describe Gitlab::Elastic::SearchResults, :elastic do
   before do
     stub_ee_application_setting(elasticsearch_search: true, elasticsearch_indexing: true)
-    Gitlab::Elastic::Helper.create_empty_index
   end
 
   after do
-    Gitlab::Elastic::Helper.delete_index
     stub_ee_application_setting(elasticsearch_search: false, elasticsearch_indexing: false)
   end
 
@@ -17,6 +15,16 @@ describe Gitlab::Elastic::SearchResults do
   let(:project_2) { create(:project, :repository, :wiki_repo) }
   let(:limit_project_ids) { [project_1.id] }
 
+  describe 'counts' do
+    it 'does not hit Elasticsearch twice for result and counts' do
+      expect(Repository).to receive(:find_commits_by_message_with_elastic).with('hello world', anything).once.and_call_original
+
+      results = described_class.new(user, 'hello world', limit_project_ids)
+      expect(results.objects('commits', 2)).to be_empty
+      expect(results.commits_count).to eq 0
+    end
+  end
+
   describe 'parse_search_result' do
     let(:blob) do
       {