Merge branch...

Merge branch '338474-dont-use-postgresql-when-repository_ref-does-not-impact-elasticsearch-query' into 'master'

Use Elasticsearch if possible for project search counts

See merge request gitlab-org/gitlab!68177

ee/app/services/ee/search/project_service.rb

@@ -6,9 +6,11 @@ module EE
       extend ::Gitlab::Utils::Override
       include ::Search::Elasticsearchable
 
+      SCOPES_THAT_SUPPORT_BRANCHES = %w(wiki_blobs commits blobs).freeze
+
       override :execute
       def execute
-        return super unless use_elasticsearch? && default_branch?
+        return super unless use_elasticsearch? && use_default_branch?
 
         if project.is_a?(Array)
           project_ids = Array(project).map(&:id)
@@ -38,8 +40,9 @@ module EE
         params[:repository_ref]
       end
 
-      def default_branch?
+      def use_default_branch?
         return true if repository_ref.blank?
+        return true unless SCOPES_THAT_SUPPORT_BRANCHES.include?(scope)
 
         project.root_ref?(repository_ref)
       end

ee/lib/gitlab/elastic/project_search_results.rb

@@ -20,7 +20,6 @@ module Gitlab
       def blobs(page: 1, per_page: DEFAULT_PER_PAGE, count_only: false, preload_method: nil)
         return Kaminari.paginate_array([]) unless Ability.allowed?(@current_user, :download_code, project)
         return Kaminari.paginate_array([]) if project.empty_repo? || query.blank?
-        return Kaminari.paginate_array([]) unless root_ref?
 
         strong_memoize(memoize_key(:blobs, count_only: count_only)) do
           project.repository.__elasticsearch__.elastic_search_as_found_blob(
@@ -35,8 +34,8 @@ module Gitlab
 
       def wiki_blobs(page: 1, per_page: DEFAULT_PER_PAGE, count_only: false)
         return Kaminari.paginate_array([]) unless Ability.allowed?(@current_user, :read_wiki, project)
+        return Kaminari.paginate_array([]) unless project.wiki_enabled? && !project.wiki.empty? && query.present?
 
-        if project.wiki_enabled? && !project.wiki.empty? && query.present?
         strong_memoize(memoize_key(:wiki_blobs, count_only: count_only)) do
           project.wiki.__elasticsearch__.elastic_search_as_wiki_page(
             query,
@@ -45,9 +44,6 @@ module Gitlab
             options: { count_only: count_only }
           )
         end
-        else
-          Kaminari.paginate_array([])
-        end
       end
 
       def notes(count_only: false)
@@ -65,12 +61,8 @@ module Gitlab
 
       def commits(page: 1, per_page: DEFAULT_PER_PAGE, preload_method: nil, count_only: false)
         return Kaminari.paginate_array([]) unless Ability.allowed?(@current_user, :download_code, project)
+        return Kaminari.paginate_array([]) if project.empty_repo? || query.blank?
 
-        if project.empty_repo? || query.blank?
-          Kaminari.paginate_array([])
-        else
-          # We use elastic for default branch only
-          if root_ref?
         strong_memoize(memoize_key(:commits, count_only: count_only)) do
           project.repository.find_commits_by_message_with_elastic(
             query,
@@ -80,20 +72,6 @@ module Gitlab
             options: { count_only: count_only }
           )
         end
-          else
-            offset = per_page * ((page || 1) - 1)
-
-            Kaminari.paginate_array(
-              project.repository.find_commits_by_message(query),
-              offset: offset,
-              limit: per_page
-            )
-          end
-        end
-      end
-
-      def root_ref?
-        project.root_ref?(repository_ref)
       end
     end
   end

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

@@ -103,65 +103,6 @@ RSpec.describe Gitlab::Elastic::ProjectSearchResults, :elastic, :clean_gitlab_re
     end
   end
 
-  describe "search for commits in non-default branch" do
-    let(:project) { create(:project, :public, :repository, visibility) }
-    let(:visibility) { :repository_enabled }
-    let(:query) { 'initial' }
-    let(:repository_ref) { 'test' }
-
-    subject(:commits) { results.objects('commits') }
-
-    it 'finds needed commit' do
-      expect(results.commits_count).to eq(1)
-    end
-
-    it 'responds to total_pages method' do
-      expect(commits.total_pages).to eq(1)
-    end
-
-    context 'disabled repository' do
-      let(:visibility) { :repository_disabled }
-
-      it 'hides commits from members' do
-        project.add_reporter(user)
-
-        is_expected.to be_empty
-      end
-
-      it 'hides commits from non-members' do
-        is_expected.to be_empty
-      end
-    end
-
-    context 'private repository' do
-      let(:visibility) { :repository_private }
-
-      it 'shows commits to members' do
-        project.add_reporter(user)
-
-        is_expected.not_to be_empty
-      end
-
-      it 'hides commits from non-members' do
-        is_expected.to be_empty
-      end
-    end
-  end
-
-  describe 'search for blobs in non-default branch' do
-    let(:project) { create(:project, :public, :repository, :repository_private) }
-    let(:query) { 'initial' }
-    let(:repository_ref) { 'test' }
-
-    subject(:blobs) { results.objects('blobs') }
-
-    it 'always returns zero results' do
-      expect_any_instance_of(Gitlab::FileFinder).to receive(:find).never
-
-      expect(blobs).to be_empty
-    end
-  end
-
   describe 'confidential issues', :sidekiq_might_not_need_inline do
     include_examples 'access restricted confidential issues' do
       before do

ee/spec/services/search/project_service_spec.rb

@@ -28,17 +28,63 @@ RSpec.describe Search::ProjectService do
     end
   end
 
-  context 'code search' do
+  context 'default branch support' do
     let(:user) { scope.owner }
     let(:scope) { create(:project) }
     let(:service) { described_class.new(scope, user, params) }
+
+    describe '#use_default_branch?' do
+      subject { service.use_default_branch? }
+
+      context 'when repository_ref param is blank' do
         let(:params) { { search: '*' } }
 
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when repository_ref param provided' do
+        let(:params) { { search: '*', scope: search_scope, repository_ref: 'test' } }
+
+        where(:search_scope, :default_branch_given, :use_default_branch) do
+          'issues'          | true   | true
+          'issues'          | false  | true
+          'merge_requests'  | true   | true
+          'merge_requests'  | false  | true
+          'notes'           | true   | true
+          'notes'           | false  | true
+          'milestones'      | true   | true
+          'milestones'      | false  | true
+          'blobs'           | true   | true
+          'blobs'           | false  | false
+          'wiki_blobs'      | true   | true
+          'wiki_blobs'      | false  | false
+          'commits'         | true   | true
+          'commits'         | false  | false
+        end
+
+        with_them do
+          before do
+            allow(scope).to receive(:root_ref?).and_return(default_branch_given)
+          end
+
+          it { is_expected.to eq(use_default_branch) }
+        end
+      end
+    end
+
     describe '#execute' do
+      let(:params) { { search: '*' } }
+
       subject { service.execute }
 
+      it 'returns Elastic results when searching non-default branch' do
+        expect(service).to receive(:use_default_branch?).and_return(true)
+
+        is_expected.to be_a(::Gitlab::Elastic::ProjectSearchResults)
+      end
+
       it 'returns ordinary results when searching non-default branch' do
-        expect(service).to receive(:default_branch?).and_return(false)
+        expect(service).to receive(:use_default_branch?).and_return(false)
 
         is_expected.to be_a(::Gitlab::ProjectSearchResults)
       end