Merge branch '216908-pass-limit-and-offset-when-searching-for-commits' into 'master'

Limit the number of results requested when searching for commits

Closes #216908

See merge request gitlab-org/gitlab!32260

changelogs/unreleased/216908-pass-limit-and-offset-when-searching-for-commits.yml

+---
+title: Improve performance of commit search by limiting the number of results requested
+merge_request: 32260
+author:
+type: performance

lib/gitlab/project_search_results.rb

@@ -20,7 +20,7 @@ module Gitlab
       when 'wiki_blobs'
         paginated_wiki_blobs(wiki_blobs(limit: limit_up_to_page(page, per_page)), page, per_page)
       when 'commits'
-        Kaminari.paginate_array(commits).page(page).per(per_page)
+        paginated_commits(page, per_page)
       when 'users'
         users.page(page).per(per_page)
       else
@@ -37,7 +37,7 @@ module Gitlab
       when 'wiki_blobs'
         wiki_blobs_count.to_s
       when 'commits'
-        commits_count.to_s
+        formatted_limited_count(commits_count)
       else
         super
       end
@@ -72,7 +72,7 @@ module Gitlab
     end
 
     def commits_count
-      @commits_count ||= commits.count
+      @commits_count ||= commits(limit: count_limit).count
     end
 
     def single_commit_result?
@@ -86,6 +86,12 @@ module Gitlab
 
     private
 
+    def paginated_commits(page, per_page)
+      results = commits(limit: limit_up_to_page(page, per_page))
+
+      Kaminari.paginate_array(results).page(page).per(per_page)
+    end
+
     def paginated_blobs(blobs, page, per_page)
       results = Kaminari.paginate_array(blobs).page(page).per(per_page)
 
@@ -139,21 +145,21 @@ module Gitlab
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
-    def commits
-      @commits ||= find_commits(query)
+    def commits(limit:)
+      @commits ||= find_commits(query, limit: limit)
     end
 
-    def find_commits(query)
+    def find_commits(query, limit:)
       return [] unless Ability.allowed?(@current_user, :download_code, @project)
 
-      commits = find_commits_by_message(query)
+      commits = find_commits_by_message(query, limit: limit)
       commit_by_sha = find_commit_by_sha(query)
       commits |= [commit_by_sha] if commit_by_sha
       commits
     end
 
-    def find_commits_by_message(query)
-      project.repository.find_commits_by_message(query)
+    def find_commits_by_message(query, limit:)
+      project.repository.find_commits_by_message(query, nil, nil, limit)
     end
 
     def find_commit_by_sha(query)

spec/lib/gitlab/project_search_results_spec.rb

@@ -34,7 +34,7 @@ describe Gitlab::ProjectSearchResults do
       'blobs'      | :limited_blobs_count    | max_limited_count
       'notes'      | :limited_notes_count    | max_limited_count
       'wiki_blobs' | :wiki_blobs_count       | '1234'
-      'commits'    | :commits_count          | '1234'
+      'commits'    | :commits_count          | max_limited_count
       'projects'   | :limited_projects_count | max_limited_count
       'unknown'    | nil                     | nil
     end
@@ -386,6 +386,19 @@ describe Gitlab::ProjectSearchResults do
     end
   end
 
+  describe '#commits_count' do
+    let(:project) { create(:project, :public, :repository) }
+
+    it 'limits the number of commits requested' do
+      expect(project.repository)
+        .to receive(:find_commits_by_message)
+        .with(anything, anything, anything, described_class::COUNT_LIMIT)
+        .and_call_original
+
+      described_class.new(user, project, '.').commits_count
+    end
+  end
+
   # Examples for commit access level test
   #
   # params:
@@ -452,6 +465,54 @@ describe Gitlab::ProjectSearchResults do
   end
 
   describe 'commit search' do
+    context 'pagination' do
+      let(:project) { create(:project, :public, :repository) }
+
+      it 'returns the correct results for each page' do
+        expect(results_page(1)).to contain_exactly(commit('b83d6e391c22777fca1ed3012fce84f633d7fed0'))
+
+        expect(results_page(2)).to contain_exactly(commit('498214de67004b1da3d820901307bed2a68a8ef6'))
+
+        expect(results_page(3)).to contain_exactly(commit('1b12f15a11fc6e62177bef08f47bc7b5ce50b141'))
+      end
+
+      it 'returns the correct number of pages' do
+        expect(results_page(1).total_pages).to eq(project.repository.commit_count)
+      end
+
+      context 'limiting requested commits' do
+        context 'on page 1' do
+          it "limits to #{described_class::COUNT_LIMIT}" do
+            expect(project.repository)
+              .to receive(:find_commits_by_message)
+              .with(anything, anything, anything, described_class::COUNT_LIMIT)
+              .and_call_original
+
+            results_page(1)
+          end
+        end
+
+        context 'on subsequent pages' do
+          it "limits to #{described_class::COUNT_LIMIT} plus page offset" do
+            expect(project.repository)
+              .to receive(:find_commits_by_message)
+              .with(anything, anything, anything, described_class::COUNT_LIMIT + 1)
+              .and_call_original
+
+            results_page(2)
+          end
+        end
+      end
+
+      def results_page(page)
+        described_class.new(user, project, '.').objects('commits', per_page: 1, page: page)
+      end
+
+      def commit(hash)
+        project.repository.commit(hash)
+      end
+    end
+
     context 'by commit message' do
       let(:project) { create(:project, :public, :repository) }
       let(:commit) { project.repository.commit('59e29889be61e6e0e5e223bfa9ac2721d31605b8') }