Merge branch 'search-100' into 'master'

Use limit for search count queries

See merge request gitlab-org/gitlab-ce!16502

13 files changed, 153 insertions, 31 deletions

diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index d2275139c42..98831f5be4a 100644
--- a/app/finders/issues_finder.rb
+++ b/app/finders/issues_finder.rb
@@ -15,6 +15,7 @@
 #     label_name: string
 #     sort: string
 #     my_reaction_emoji: string
+#     public_only: boolean
 #
 class IssuesFinder < IssuableFinder
   CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER
@@ -40,7 +41,15 @@ class IssuesFinder < IssuableFinder
   private
 
   def init_collection
-    with_confidentiality_access_check
+    if public_only?
+      Issue.public_only
+    else
+      with_confidentiality_access_check
+    end
+  end
+
+  def public_only?
+    params.fetch(:public_only, false)
   end
 
   def user_can_see_all_confidential_issues?
diff --git a/app/helpers/search_helper.rb b/app/helpers/search_helper.rb
index 0f9ac958f95..e6a6496871a 100644
--- a/app/helpers/search_helper.rb
+++ b/app/helpers/search_helper.rb
@@ -170,4 +170,8 @@ module SearchHelper
     # Truncato's filtered_tags and filtered_attributes are not quite the same
     sanitize(html, tags: %w(a p ol ul li pre code))
   end
+
+  def limited_count(count, limit = 1000)
+    count > limit ? "#{limit}+" : count
+  end
 end
diff --git a/app/views/search/_category.html.haml b/app/views/search/_category.html.haml
index 314d8e9cb25..915e648a5d3 100644
--- a/app/views/search/_category.html.haml
+++ b/app/views/search/_category.html.haml
@@ -57,25 +57,24 @@
           Titles and Filenames
           %span.badge
             = @search_results.snippet_titles_count
-
     - else
       %li{ class: active_when(@scope == 'projects') }
         = link_to search_filter_path(scope: 'projects') do
           Projects
           %span.badge
-            = @search_results.projects_count
+            = limited_count(@search_results.limited_projects_count)
       %li{ class: active_when(@scope == 'issues') }
         = link_to search_filter_path(scope: 'issues') do
           Issues
           %span.badge
-            = @search_results.issues_count
+            = limited_count(@search_results.limited_issues_count)
       %li{ class: active_when(@scope == 'merge_requests') }
         = link_to search_filter_path(scope: 'merge_requests') do
           Merge requests
           %span.badge
-            = @search_results.merge_requests_count
+            = limited_count(@search_results.limited_merge_requests_count)
       %li{ class: active_when(@scope == 'milestones') }
         = link_to search_filter_path(scope: 'milestones') do
           Milestones
           %span.badge
-            = @search_results.milestones_count
+            = limited_count(@search_results.limited_milestones_count)
diff --git a/app/views/search/_results.html.haml b/app/views/search/_results.html.haml
index 02133d09cdf..60ef44482f0 100644
--- a/app/views/search/_results.html.haml
+++ b/app/views/search/_results.html.haml
@@ -2,7 +2,8 @@
   = render partial: "search/results/empty"
 - else
   .row-content-block
-    = search_entries_info(@search_objects, @scope, @search_term)
+    - unless @search_objects.is_a?(Kaminari::PaginatableWithoutCount)
+      = search_entries_info(@search_objects, @scope, @search_term)
     - unless @show_snippets
       - if @project
         in project #{link_to @project.name_with_namespace, [@project.namespace.becomes(Namespace), @project]}
@@ -22,4 +23,4 @@
           = render partial: "search/results/#{@scope.singularize}", collection: @search_objects
 
     - if @scope != 'projects'
-      = paginate(@search_objects, theme: 'gitlab')
+      = paginate_collection(@search_objects)
diff --git a/changelogs/unreleased/40540-use-limit-for-global-search.yml b/changelogs/unreleased/40540-use-limit-for-global-search.yml
new file mode 100644
index 00000000000..7d9612c16df
--- /dev/null
+++ b/changelogs/unreleased/40540-use-limit-for-global-search.yml
@@ -0,0 +1,5 @@
+---
+title: Optimize search queries on the search page by setting a limit for matching records.
+merge_request:
+author:
+type: performance
diff --git a/db/migrate/20180115201419_add_index_updated_at_to_issues.rb b/db/migrate/20180115201419_add_index_updated_at_to_issues.rb
new file mode 100644
index 00000000000..a5a48fc97be
--- /dev/null
+++ b/db/migrate/20180115201419_add_index_updated_at_to_issues.rb
@@ -0,0 +1,15 @@
+class AddIndexUpdatedAtToIssues < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :issues, :updated_at
+  end
+
+  def down
+    remove_concurrent_index :issues, :updated_at
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index a0901833c3d..4e82a688725 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180113220114) do
+ActiveRecord::Schema.define(version: 20180115201419) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -886,6 +886,7 @@ ActiveRecord::Schema.define(version: 20180113220114) do
   add_index "issues", ["relative_position"], name: "index_issues_on_relative_position", using: :btree
   add_index "issues", ["state"], name: "index_issues_on_state", using: :btree
   add_index "issues", ["title"], name: "index_issues_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"}
+  add_index "issues", ["updated_at"], name: "index_issues_on_updated_at", using: :btree
   add_index "issues", ["updated_by_id"], name: "index_issues_on_updated_by_id", where: "(updated_by_id IS NOT NULL)", using: :btree
 
   create_table "keys", force: :cascade do |t|
diff --git a/lib/api/v3/projects.rb b/lib/api/v3/projects.rb
index 446f804124b..a7f0813bf74 100644
--- a/lib/api/v3/projects.rb
+++ b/lib/api/v3/projects.rb
@@ -175,7 +175,7 @@ module API
         end
         get "/search/:query", requirements: { query: /[^\/]+/ } do
           search_service = Search::GlobalService.new(current_user, search: params[:query]).execute
-          projects = search_service.objects('projects', params[:page])
+          projects = search_service.objects('projects', params[:page], false)
           projects = projects.reorder(params[:order_by] => params[:sort])
 
           present paginate(projects), with: ::API::V3::Entities::Project
diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb
index 7771b15069b..4823f703ba4 100644
--- a/lib/gitlab/project_search_results.rb
+++ b/lib/gitlab/project_search_results.rb
@@ -20,7 +20,7 @@ module Gitlab
       when 'commits'
         Kaminari.paginate_array(commits).page(page).per(per_page)
       else
-        super
+        super(scope, page, false)
       end
     end
 
diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb
index 70b639501fd..7362514167f 100644
--- a/lib/gitlab/search_results.rb
+++ b/lib/gitlab/search_results.rb
@@ -40,19 +40,21 @@ module Gitlab
       @default_project_filter = default_project_filter
     end
 
-    def objects(scope, page = nil)
-      case scope
-      when 'projects'
-        projects.page(page).per(per_page)
-      when 'issues'
-        issues.page(page).per(per_page)
-      when 'merge_requests'
-        merge_requests.page(page).per(per_page)
-      when 'milestones'
-        milestones.page(page).per(per_page)
-      else
-        Kaminari.paginate_array([]).page(page).per(per_page)
-      end
+    def objects(scope, page = nil, without_count = true)
+      collection = case scope
+                   when 'projects'
+                     projects.page(page).per(per_page)
+                   when 'issues'
+                     issues.page(page).per(per_page)
+                   when 'merge_requests'
+                     merge_requests.page(page).per(per_page)
+                   when 'milestones'
+                     milestones.page(page).per(per_page)
+                   else
+                     Kaminari.paginate_array([]).page(page).per(per_page)
+                   end
+
+      without_count ? collection.without_count : collection
     end
 
     def projects_count
@@ -71,18 +73,46 @@ module Gitlab
       @milestones_count ||= milestones.count
     end
 
+    def limited_projects_count
+      @limited_projects_count ||= projects.limit(count_limit).count
+    end
+
+    def limited_issues_count
+      return @limited_issues_count if @limited_issues_count
+
+      # By default getting limited count (e.g. 1000+) is fast on issuable
+      # collections except for issues, where filtering both not confidential
+      # and confidential issues user has access to, is too complex.
+      # It's faster to try to fetch all public issues first, then only
+      # if necessary try to fetch all issues.
+      sum = issues(public_only: true).limit(count_limit).count
+      @limited_issues_count = sum < count_limit ? issues.limit(count_limit).count : sum
+    end
+
+    def limited_merge_requests_count
+      @limited_merge_requests_count ||= merge_requests.limit(count_limit).count
+    end
+
+    def limited_milestones_count
+      @limited_milestones_count ||= milestones.limit(count_limit).count
+    end
+
     def single_commit_result?
       false
     end
 
+    def count_limit
+      1001
+    end
+
     private
 
     def projects
       limit_projects.search(query)
     end
 
-    def issues
-      issues = IssuesFinder.new(current_user).execute
+    def issues(finder_params = {})
+      issues = IssuesFinder.new(current_user, finder_params).execute
       unless default_project_filter
         issues = issues.where(project_id: project_ids_relation)
       end
@@ -94,13 +124,13 @@ module Gitlab
           issues.full_search(query)
         end
 
-      issues.order('updated_at DESC')
+      issues.reorder('updated_at DESC')
     end
 
     def milestones
       milestones = Milestone.where(project_id: project_ids_relation)
       milestones = milestones.search(query)
-      milestones.order('updated_at DESC')
+      milestones.reorder('updated_at DESC')
     end
 
     def merge_requests
@@ -116,7 +146,7 @@ module Gitlab
           merge_requests.full_search(query)
         end
 
-      merge_requests.order('updated_at DESC')
+      merge_requests.reorder('updated_at DESC')
     end
 
     def default_scope
diff --git a/lib/gitlab/snippet_search_results.rb b/lib/gitlab/snippet_search_results.rb
index b85f70e450e..4f86b3e8f73 100644
--- a/lib/gitlab/snippet_search_results.rb
+++ b/lib/gitlab/snippet_search_results.rb
@@ -16,7 +16,7 @@ module Gitlab
       when 'snippet_blobs'
         snippet_blobs.page(page).per(per_page)
       else
-        super
+        super(scope, nil, false)
       end
     end
 
diff --git a/spec/features/global_search_spec.rb b/spec/features/global_search_spec.rb
index 4f575613848..f8c4db1403c 100644
--- a/spec/features/global_search_spec.rb
+++ b/spec/features/global_search_spec.rb
@@ -22,7 +22,7 @@ feature 'Global search' do
       click_button "Go"
 
       select_filter("Issues")
-      expect(page).to have_selector('.gl-pagination .page', count: 2)
+      expect(page).to have_selector('.gl-pagination .next')
     end
   end
 end
diff --git a/spec/lib/gitlab/search_results_spec.rb b/spec/lib/gitlab/search_results_spec.rb
index b5a9ac570e6..17b48b3d062 100644
--- a/spec/lib/gitlab/search_results_spec.rb
+++ b/spec/lib/gitlab/search_results_spec.rb
@@ -19,6 +19,12 @@ describe Gitlab::SearchResults do
       project.add_developer(user)
     end
 
+    describe '#objects' do
+      it 'returns without_page collection by default' do
+        expect(results.objects('projects')).to be_kind_of(Kaminari::PaginatableWithoutCount)
+      end
+    end
+
     describe '#projects_count' do
       it 'returns the total amount of projects' do
         expect(results.projects_count).to eq(1)
@@ -43,6 +49,58 @@ describe Gitlab::SearchResults do
       end
     end
 
+    context "when count_limit is lower than total amount" do
+      before do
+        allow(results).to receive(:count_limit).and_return(1)
+      end
+
+      describe '#limited_projects_count' do
+        it 'returns the limited amount of projects' do
+          create(:project, name: 'foo2')
+
+          expect(results.limited_projects_count).to eq(1)
+        end
+      end
+
+      describe '#limited_merge_requests_count' do
+        it 'returns the limited amount of merge requests' do
+          create(:merge_request, :simple, source_project: project, title: 'foo2')
+
+          expect(results.limited_merge_requests_count).to eq(1)
+        end
+      end
+
+      describe '#limited_milestones_count' do
+        it 'returns the limited amount of milestones' do
+          create(:milestone, project: project, title: 'foo2')
+
+          expect(results.limited_milestones_count).to eq(1)
+        end
+      end
+
+      describe '#limited_issues_count' do
+        it 'runs single SQL query to get the limited amount of issues' do
+          create(:milestone, project: project, title: 'foo2')
+
+          expect(results).to receive(:issues).with(public_only: true).and_call_original
+          expect(results).not_to receive(:issues).with(no_args).and_call_original
+
+          expect(results.limited_issues_count).to eq(1)
+        end
+      end
+    end
+
+    context "when count_limit is higher than total amount" do
+      describe '#limited_issues_count' do
+        it 'runs multiple queries to get the limited amount of issues' do
+          expect(results).to receive(:issues).with(public_only: true).and_call_original
+          expect(results).to receive(:issues).with(no_args).and_call_original
+
+          expect(results.limited_issues_count).to eq(1)
+        end
+      end
+    end
+
     it 'includes merge requests from source and target projects' do
       forked_project = fork_project(project, user)
       merge_request_2 = create(:merge_request, target_project: project, source_project: forked_project, title: 'foo')