From 3be98bd28e757cb7bd60aa435ea5d0acf122ab56 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Fri, 3 Jul 2020 17:12:29 +0930
Subject: [PATCH 1/7] Use gitaly pagination to improve performance

Pagination by Kaminari is slow because it requests all branches and
paginate after receiving results from Gitaly. It's a first iteration to
implement native pagination with Gitaly.
---
 app/finders/branches_finder.rb                |  24 ++
 app/models/repository.rb                      |   4 +-
 ...e-of-branches-list-api-when-under-load.yml |   5 +
 lib/api/branches.rb                           |  11 +-
 lib/gitlab/git/repository.rb                  |   4 +-
 lib/gitlab/gitaly_client/ref_service.rb       |   4 +-
 spec/finders/branches_finder_spec.rb          | 271 +++++++++++++-----
 spec/requests/api/branches_spec.rb            | 107 +++++--
 8 files changed, 327 insertions(+), 103 deletions(-)
 create mode 100644 changelogs/unreleased/208738-improve-performance-of-branches-list-api-when-under-load.yml

diff --git a/app/finders/branches_finder.rb b/app/finders/branches_finder.rb
index 8001c70a9b2bad..0f275df065e357 100644
--- a/app/finders/branches_finder.rb
+++ b/app/finders/branches_finder.rb
@@ -12,12 +12,36 @@ def execute
     branches
   end
 
+  def with_gitaly_pagination
+    if names || search
+      execute
+    else
+      repository.branches_sorted_by(sort, pagination_params)
+    end
+  end
+
   private
 
   def names
     @params[:names].presence
   end
 
+  def per_page
+    @params[:per_page].presence
+  end
+
+  def page_token
+    @params[:page_token].presence
+  end
+
+  def branch_ref
+    Gitlab::Git::BRANCH_REF_PREFIX + page_token if page_token
+  end
+
+  def pagination_params
+    { limit: per_page, page_token: branch_ref } if per_page || page_token
+  end
+
   def by_names(branches)
     return branches unless names
 
diff --git a/app/models/repository.rb b/app/models/repository.rb
index 6d167700ff4cf2..48e96d4c193d25 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -713,8 +713,8 @@ def next_branch(name, opts = {})
     "#{name}-#{highest_branch_id + 1}"
   end
 
-  def branches_sorted_by(value)
-    raw_repository.local_branches(sort_by: value)
+  def branches_sorted_by(sort_by, pagination_params = nil)
+    raw_repository.local_branches(sort_by: sort_by, pagination_params: pagination_params)
   end
 
   def tags_sorted_by(value)
diff --git a/changelogs/unreleased/208738-improve-performance-of-branches-list-api-when-under-load.yml b/changelogs/unreleased/208738-improve-performance-of-branches-list-api-when-under-load.yml
new file mode 100644
index 00000000000000..90f90e285e8786
--- /dev/null
+++ b/changelogs/unreleased/208738-improve-performance-of-branches-list-api-when-under-load.yml
@@ -0,0 +1,5 @@
+---
+title: Use native Gitaly pagination for Branch list API
+merge_request: 35819
+author:
+type: changed
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index bbed50e96ea310..da1aac41f3e565 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -32,14 +32,21 @@ class Branches < Grape::API::Instance
       params do
         use :pagination
         use :filter_params
+
+        optional :page_token, type: String, desc: 'Name of branch to start the paginaition from'
       end
       get ':id/repository/branches' do
         user_project.preload_protected_branches
 
         repository = user_project.repository
 
-        branches = BranchesFinder.new(repository, declared_params(include_missing: false)).execute
-        branches = paginate(::Kaminari.paginate_array(branches))
+        if Feature.enabled?(:gitaly_branch_pagination, user_project)
+          branches = BranchesFinder.new(repository, declared_params(include_missing: false)).with_gitaly_pagination
+        else
+          branches = BranchesFinder.new(repository, declared_params(include_missing: false)).execute
+          branches = paginate(::Kaminari.paginate_array(branches))
+        end
+
         merged_branch_names = repository.merged_branch_names(branches.map(&:name))
 
         present(
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index a1095ff9a86010..ea7a6e84195cbf 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -127,9 +127,9 @@ def find_branch(name)
         end
       end
 
-      def local_branches(sort_by: nil)
+      def local_branches(sort_by: nil, pagination_params: nil)
         wrapped_gitaly_errors do
-          gitaly_ref_client.local_branches(sort_by: sort_by)
+          gitaly_ref_client.local_branches(sort_by: sort_by, pagination_params: pagination_params)
         end
       end
 
diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb
index e0ccc13234ad4a..97b6813c080d37 100644
--- a/lib/gitlab/gitaly_client/ref_service.rb
+++ b/lib/gitlab/gitaly_client/ref_service.rb
@@ -110,8 +110,8 @@ def count_branch_names
         branch_names.count
       end
 
-      def local_branches(sort_by: nil)
-        request = Gitaly::FindLocalBranchesRequest.new(repository: @gitaly_repo)
+      def local_branches(sort_by: nil, pagination_params: nil)
+        request = Gitaly::FindLocalBranchesRequest.new(repository: @gitaly_repo, pagination_params: pagination_params)
         request.sort_by = sort_by_param(sort_by) if sort_by
         response = GitalyClient.call(@storage, :ref_service, :find_local_branches, request, timeout: GitalyClient.fast_timeout)
         consume_find_local_branches_response(response)
diff --git a/spec/finders/branches_finder_spec.rb b/spec/finders/branches_finder_spec.rb
index 2e52093342d104..5273f82fc5a9d9 100644
--- a/spec/finders/branches_finder_spec.rb
+++ b/spec/finders/branches_finder_spec.rb
@@ -7,142 +7,261 @@
   let(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
 
-  describe '#execute' do
+  let(:branch_finder) { described_class.new(repository, params) }
+  let(:params) { {} }
+
+  shared_examples 'default branch finder' do
     context 'sort only' do
-      it 'sorts by name' do
-        branches_finder = described_class.new(repository, {})
+      context 'by name' do
+        let(:params) { {} }
 
-        result = branches_finder.execute
+        it 'sorts' do
+          result = subject
 
-        expect(result.first.name).to eq("'test'")
+          expect(result.first.name).to eq("'test'")
+        end
       end
 
-      it 'sorts by recently_updated' do
-        branches_finder = described_class.new(repository, { sort: 'updated_desc' })
+      context 'by recently_updated' do
+        let(:params) { { sort: 'updated_desc' } }
 
-        result = branches_finder.execute
+        it 'sorts' do
+          result = subject
 
-        recently_updated_branch = repository.branches.max do |a, b|
-          repository.commit(a.dereferenced_target).committed_date <=> repository.commit(b.dereferenced_target).committed_date
-        end
+          recently_updated_branch = repository.branches.max do |a, b|
+            repository.commit(a.dereferenced_target).committed_date <=> repository.commit(b.dereferenced_target).committed_date
+          end
 
-        expect(result.first.name).to eq(recently_updated_branch.name)
+          expect(result.first.name).to eq(recently_updated_branch.name)
+        end
       end
 
-      it 'sorts by last_updated' do
-        branches_finder = described_class.new(repository, { sort: 'updated_asc' })
+      context 'by last_updated' do
+        let(:params) { { sort: 'updated_asc' } }
 
-        result = branches_finder.execute
+        it 'sorts' do
+          result = subject
 
-        expect(result.first.name).to eq('feature')
+          expect(result.first.name).to eq('feature')
+        end
       end
     end
 
     context 'filter only' do
-      it 'filters branches by name' do
-        branches_finder = described_class.new(repository, { search: 'fix' })
+      context 'by name' do
+        let(:params) { { search: 'fix' } }
 
-        result = branches_finder.execute
+        it 'filters branches' do
+          result = subject
 
-        expect(result.first.name).to eq('fix')
-        expect(result.count).to eq(1)
+          expect(result.first.name).to eq('fix')
+          expect(result.count).to eq(1)
+        end
       end
 
-      it 'filters branches by name ignoring letter case' do
-        branches_finder = described_class.new(repository, { search: 'FiX' })
+      context 'by name ignoring letter case' do
+        let(:params) { { search: 'FiX' } }
 
-        result = branches_finder.execute
+        it 'filters branches' do
+          result = subject
 
-        expect(result.first.name).to eq('fix')
-        expect(result.count).to eq(1)
+          expect(result.first.name).to eq('fix')
+          expect(result.count).to eq(1)
+        end
       end
 
-      it 'does not find any branch with that name' do
-        branches_finder = described_class.new(repository, { search: 'random' })
+      context 'with an unknown name' do
+        let(:params) { { search: 'random' } }
 
-        result = branches_finder.execute
+        it 'does not find any branch' do
+          result = subject
 
-        expect(result.count).to eq(0)
+          expect(result.count).to eq(0)
+        end
       end
 
-      it 'filters branches by provided names' do
-        branches_finder = described_class.new(repository, { names: %w[fix csv lfs does-not-exist] })
+      context 'by provided names' do
+        let(:params) { { names: %w[fix csv lfs does-not-exist] } }
 
-        result = branches_finder.execute
+        it 'filters branches' do
+          result = subject
 
-        expect(result.count).to eq(3)
-        expect(result.map(&:name)).to eq(%w{csv fix lfs})
+          expect(result.count).to eq(3)
+          expect(result.map(&:name)).to eq(%w{csv fix lfs})
+        end
       end
 
-      it 'filters branches by name that begins with' do
-        params = { search: '^feature_' }
-        branches_finder = described_class.new(repository, params)
+      context 'by name that begins with' do
+        let(:params) { { search: '^feature_' } }
 
-        result = branches_finder.execute
+        it 'filters branches' do
+          result = subject
 
-        expect(result.first.name).to eq('feature_conflict')
-        expect(result.count).to eq(1)
+          expect(result.first.name).to eq('feature_conflict')
+          expect(result.count).to eq(1)
+        end
       end
 
-      it 'filters branches by name that ends with' do
-        params = { search: 'feature$' }
-        branches_finder = described_class.new(repository, params)
+      context 'by name that ends with' do
+        let(:params) { { search: 'feature$' } }
 
-        result = branches_finder.execute
+        it 'filters branches' do
+          result = subject
 
-        expect(result.first.name).to eq('feature')
-        expect(result.count).to eq(1)
+          expect(result.first.name).to eq('feature')
+          expect(result.count).to eq(1)
+        end
       end
 
-      it 'filters branches by nonexistent name that begins with' do
-        params = { search: '^nope' }
-        branches_finder = described_class.new(repository, params)
+      context 'by nonexistent name that begins with' do
+        let(:params) { { search: '^nope' } }
 
-        result = branches_finder.execute
+        it 'filters branches' do
+          result = subject
 
-        expect(result.count).to eq(0)
+          expect(result.count).to eq(0)
+        end
       end
 
-      it 'filters branches by nonexistent name that ends with' do
-        params = { search: 'nope$' }
-        branches_finder = described_class.new(repository, params)
+      context 'by nonexistent name that ends with' do
+        let(:params) { { search: 'nope$' } }
 
-        result = branches_finder.execute
+        it 'filters branches' do
+          result = subject
 
-        expect(result.count).to eq(0)
+          expect(result.count).to eq(0)
+        end
       end
     end
 
     context 'filter and sort' do
-      it 'filters branches by name and sorts by recently_updated' do
-        params = { sort: 'updated_desc', search: 'feat' }
-        branches_finder = described_class.new(repository, params)
+      context 'by name and sorts by recently_updated' do
+        let(:params) { { sort: 'updated_desc', search: 'feat' } }
 
-        result = branches_finder.execute
+        it 'filters branches' do
+          result = subject
 
-        expect(result.first.name).to eq('feature_conflict')
-        expect(result.count).to eq(2)
+          expect(result.first.name).to eq('feature_conflict')
+          expect(result.count).to eq(2)
+        end
       end
 
-      it 'filters branches by name and sorts by recently_updated, with exact matches first' do
-        params = { sort: 'updated_desc', search: 'feature' }
-        branches_finder = described_class.new(repository, params)
+      context 'by name and sorts by recently_updated, with exact matches first' do
+        let(:params) { { sort: 'updated_desc', search: 'feature' } }
 
-        result = branches_finder.execute
+        it 'filters branches' do
+          result = subject
 
-        expect(result.first.name).to eq('feature')
-        expect(result.second.name).to eq('feature_conflict')
-        expect(result.count).to eq(2)
+          expect(result.first.name).to eq('feature')
+          expect(result.second.name).to eq('feature_conflict')
+          expect(result.count).to eq(2)
+        end
       end
 
-      it 'filters branches by name and sorts by last_updated' do
-        params = { sort: 'updated_asc', search: 'feature' }
-        branches_finder = described_class.new(repository, params)
+      context 'by name and sorts by last_updated' do
+        let(:params) { { sort: 'updated_asc', search: 'feature' } }
+
+        it 'filters branches' do
+          result = subject
+
+          expect(result.first.name).to eq('feature')
+          expect(result.count).to eq(2)
+        end
+      end
+    end
+  end
+
+  describe '#execute' do
+    subject { branch_finder.execute }
+
+    it_behaves_like 'default branch finder'
+  end
+
+  describe '#with_gitaly_pagination' do
+    subject { branch_finder.with_gitaly_pagination }
+
+    it_behaves_like 'default branch finder'
+
+    context 'by page_token and per_page' do
+      let(:params) { { page_token: 'feature', per_page: 2 } }
+
+      it 'filters branches' do
+        result = subject
+
+        expect(result.map(&:name)).to eq(%w(feature_conflict fix))
+      end
+    end
+
+    context 'by next page_token and per_page' do
+      let(:params) { { page_token: 'fix', per_page: 2 } }
+
+      it 'filters branches' do
+        result = subject
+
+        expect(result.map(&:name)).to eq(%w(flatten-dir gitattributes))
+      end
+    end
+
+    context 'by per_page only' do
+      let(:params) { { per_page: 2 } }
+
+      it 'filters branches' do
+        result = subject
+
+        expect(result.map(&:name)).to eq(["'test'", '2-mb-file'])
+      end
+    end
+
+    context 'by page_token only' do
+      let(:params) { { page_token: 'feature' } }
+
+      it 'returns nothing' do
+        result = subject
+
+        expect(result.count).to eq(0)
+      end
+    end
+
+    context 'pagination and sort' do
+      context 'by per_page' do
+        let(:params) { { sort: 'updated_asc', per_page: 5 } }
+
+        it 'filters branches' do
+          result = subject
+
+          expect(result.map(&:name)).to eq(%w(feature improve/awesome merge-test markdown feature_conflict))
+        end
+      end
+
+      context 'by page_token and per_page' do
+        let(:params) { { sort: 'updated_asc', page_token: 'improve/awesome', per_page: 2 } }
+
+        it 'filters branches' do
+          result = subject
+
+          expect(result.map(&:name)).to eq(%w(merge-test markdown))
+        end
+      end
+    end
+
+    context 'pagination and names' do
+      let(:params) { { page_token: 'fix', per_page: 2, names: %w[fix csv lfs does-not-exist] } }
+
+      it 'falls back to default execute and ignore paginations' do
+        result = subject
+
+        expect(result.count).to eq(3)
+        expect(result.map(&:name)).to eq(%w{csv fix lfs})
+      end
+    end
+
+    context 'pagination and search' do
+      let(:params) { { page_token: 'feature', per_page: 2, search: '^f' } }
 
-        result = branches_finder.execute
+      it 'falls back to default execute and ignore paginations' do
+        result = subject
 
-        expect(result.first.name).to eq('feature')
-        expect(result.count).to eq(2)
+        expect(result.map(&:name)).to eq(%w(feature feature_conflict fix flatten-dir))
       end
     end
   end
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index db017f8e1af8c9..d08fbe01e8c32c 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -17,6 +17,7 @@
   before do
     project.add_maintainer(user)
     project.repository.add_branch(user, 'ends-with.txt', branch_sha)
+    stub_feature_flags(gitaly_branch_pagination: false)
   end
 
   describe "GET /projects/:id/repository/branches" do
@@ -29,16 +30,6 @@
         end
       end
 
-      it 'returns the repository branches' do
-        get api(route, current_user), params: { per_page: 100 }
-
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(response).to match_response_schema('public_api/v4/branches')
-        expect(response).to include_pagination_headers
-        branch_names = json_response.map { |x| x['name'] }
-        expect(branch_names).to match_array(project.repository.branch_names)
-      end
-
       def check_merge_status(json_response)
         merged, unmerged = json_response.partition { |branch| branch['merged'] }
         merged_branches = merged.map { |branch| branch['name'] }
@@ -47,22 +38,100 @@ def check_merge_status(json_response)
         expect(project.repository.merged_branch_names(unmerged_branches)).to be_empty
       end
 
-      it 'determines only a limited number of merged branch names' do
-        expect(API::Entities::Branch).to receive(:represent).with(anything, has_up_to_merged_branch_names_count(2)).and_call_original
+      context 'with gitaly_branch_pagination feture off' do
+        context 'with legacy pagination params' do
+          it 'returns the repository branches' do
+            get api(route, current_user), params: { per_page: 100 }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(response).to match_response_schema('public_api/v4/branches')
+            expect(response).to include_pagination_headers
+            branch_names = json_response.map { |x| x['name'] }
+            expect(branch_names).to match_array(project.repository.branch_names)
+          end
 
-        get api(route, current_user), params: { per_page: 2 }
+          it 'determines only a limited number of merged branch names' do
+            expect(API::Entities::Branch).to receive(:represent).with(anything, has_up_to_merged_branch_names_count(2)).and_call_original
 
-        expect(response).to have_gitlab_http_status(:ok)
+            get api(route, current_user), params: { per_page: 2 }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.count).to eq 2
+
+            check_merge_status(json_response)
+          end
+
+          it 'merge status matches reality on paginated input' do
+            get api(route, current_user), params: { per_page: 20, page: 2 }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.count).to eq 20
+            expect(json_response.first['name']).to eq('conflict-start')
+
+            check_merge_status(json_response)
+          end
+        end
+
+        context 'with gitaly pagination params ' do
+          it 'merge status matches reality on paginated input' do
+            get api(route, current_user), params: { per_page: 20, page_token: 'feature' }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.count).to eq 20
+            expect(json_response.first['name']).to eq("'test'")
 
-        check_merge_status(json_response)
+            check_merge_status(json_response)
+          end
+        end
       end
 
-      it 'merge status matches reality on paginated input' do
-        get api(route, current_user), params: { per_page: 20, page: 2 }
+      context 'with gitaly_branch_pagination feture on' do
+        before do
+          stub_feature_flags(gitaly_branch_pagination: true)
+        end
 
-        expect(response).to have_gitlab_http_status(:ok)
+        context 'with gitaly pagination params ' do
+          it 'returns the repository branches' do
+            get api(route, current_user), params: { per_page: 100 }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(response).to match_response_schema('public_api/v4/branches')
+            branch_names = json_response.map { |x| x['name'] }
+            expect(branch_names).to match_array(project.repository.branch_names)
+          end
+
+          it 'determines only a limited number of merged branch names' do
+            expect(API::Entities::Branch).to receive(:represent).with(anything, has_up_to_merged_branch_names_count(2)).and_call_original
+
+            get api(route, current_user), params: { per_page: 2 }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.count).to eq 2
+
+            check_merge_status(json_response)
+          end
 
-        check_merge_status(json_response)
+          it 'merge status matches reality on paginated input' do
+            get api(route, current_user), params: { per_page: 20, page_token: 'feature' }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.count).to eq 20
+            expect(json_response.first['name']).to eq('feature_conflict')
+
+            check_merge_status(json_response)
+          end
+        end
+
+        context 'with legacy pagination params' do
+          it 'merge status matches reality on paginated input' do
+            get api(route, current_user), params: { per_page: 20, page: 2 }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response.first['name']).to eq("'test'")
+
+            check_merge_status(json_response)
+          end
+        end
       end
 
       context 'when repository is disabled' do
-- 
GitLab


From 530bdb40e48d027a272a0026fc1ce8194cd71dc9 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Mon, 13 Jul 2020 06:16:59 +0000
Subject: [PATCH 2/7] Apply 1 suggestion(s) to 1 file(s)

---
 spec/requests/api/branches_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index d08fbe01e8c32c..d039f4f9b3fea1 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -85,7 +85,7 @@ def check_merge_status(json_response)
         end
       end
 
-      context 'with gitaly_branch_pagination feture on' do
+      context 'with gitaly_branch_pagination feature on' do
         before do
           stub_feature_flags(gitaly_branch_pagination: true)
         end
-- 
GitLab


From c15e19f62f4edf78f1c088f6e30ea38372adb0ce Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Mon, 13 Jul 2020 07:01:56 +0000
Subject: [PATCH 3/7] Apply 1 suggestion(s) to 1 file(s)

---
 spec/requests/api/branches_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index d039f4f9b3fea1..ef3fca1c23a72f 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -38,7 +38,7 @@ def check_merge_status(json_response)
         expect(project.repository.merged_branch_names(unmerged_branches)).to be_empty
       end
 
-      context 'with gitaly_branch_pagination feture off' do
+      context 'with gitaly_branch_pagination feature off' do
         context 'with legacy pagination params' do
           it 'returns the repository branches' do
             get api(route, current_user), params: { per_page: 100 }
-- 
GitLab


From 641a570cd02d0b4981b40f8c53a0d14caa7dada4 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Mon, 13 Jul 2020 07:39:43 +0000
Subject: [PATCH 4/7] Apply 1 suggestion(s) to 1 file(s)

---
 app/finders/branches_finder.rb | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/app/finders/branches_finder.rb b/app/finders/branches_finder.rb
index 0f275df065e357..0408bf82ca290e 100644
--- a/app/finders/branches_finder.rb
+++ b/app/finders/branches_finder.rb
@@ -31,11 +31,7 @@ def per_page
   end
 
   def page_token
-    @params[:page_token].presence
-  end
-
-  def branch_ref
-    Gitlab::Git::BRANCH_REF_PREFIX + page_token if page_token
+    "#{Gitlab::Git::BRANCH_REF_PREFIX}#{@params[:page_token]}" if @params[:page_token]
   end
 
   def pagination_params
-- 
GitLab


From aa2bc1ac05e882585573ae73a5578fc7ab4fb8f2 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Tue, 14 Jul 2020 19:24:45 +0930
Subject: [PATCH 5/7] Apply suggestions

---
 app/finders/branches_finder.rb       |  20 ++---
 lib/api/branches.rb                  |   2 +-
 spec/finders/branches_finder_spec.rb | 116 +++++++++++++--------------
 spec/requests/api/branches_spec.rb   |  17 ++--
 4 files changed, 76 insertions(+), 79 deletions(-)

diff --git a/app/finders/branches_finder.rb b/app/finders/branches_finder.rb
index 0408bf82ca290e..4deada3a7de5fe 100644
--- a/app/finders/branches_finder.rb
+++ b/app/finders/branches_finder.rb
@@ -5,18 +5,14 @@ def initialize(repository, params = {})
     super(repository, params)
   end
 
-  def execute
-    branches = repository.branches_sorted_by(sort)
-    branches = by_search(branches)
-    branches = by_names(branches)
-    branches
-  end
-
-  def with_gitaly_pagination
-    if names || search
-      execute
-    else
+  def execute(gitaly_pagination = false)
+    if gitaly_pagination && names.blank? && search.blank?
       repository.branches_sorted_by(sort, pagination_params)
+    else
+      branches = repository.branches_sorted_by(sort)
+      branches = by_search(branches)
+      branches = by_names(branches)
+      branches
     end
   end
 
@@ -35,7 +31,7 @@ def page_token
   end
 
   def pagination_params
-    { limit: per_page, page_token: branch_ref } if per_page || page_token
+    { limit: per_page, page_token: page_token }
   end
 
   def by_names(branches)
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index da1aac41f3e565..7cada2318d9eaf 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -41,7 +41,7 @@ class Branches < Grape::API::Instance
         repository = user_project.repository
 
         if Feature.enabled?(:gitaly_branch_pagination, user_project)
-          branches = BranchesFinder.new(repository, declared_params(include_missing: false)).with_gitaly_pagination
+          branches = BranchesFinder.new(repository, declared_params(include_missing: false)).execute(true)
         else
           branches = BranchesFinder.new(repository, declared_params(include_missing: false)).execute
           branches = paginate(::Kaminari.paginate_array(branches))
diff --git a/spec/finders/branches_finder_spec.rb b/spec/finders/branches_finder_spec.rb
index 5273f82fc5a9d9..b61e33235d548c 100644
--- a/spec/finders/branches_finder_spec.rb
+++ b/spec/finders/branches_finder_spec.rb
@@ -10,7 +10,9 @@
   let(:branch_finder) { described_class.new(repository, params) }
   let(:params) { {} }
 
-  shared_examples 'default branch finder' do
+  describe '#execute' do
+    subject { branch_finder.execute }
+
     context 'sort only' do
       context 'by name' do
         let(:params) { {} }
@@ -169,99 +171,91 @@
         end
       end
     end
-  end
 
-  describe '#execute' do
-    subject { branch_finder.execute }
+    context 'with gitaly pagination' do
+      subject { branch_finder.execute(true) }
 
-    it_behaves_like 'default branch finder'
-  end
-
-  describe '#with_gitaly_pagination' do
-    subject { branch_finder.with_gitaly_pagination }
-
-    it_behaves_like 'default branch finder'
-
-    context 'by page_token and per_page' do
-      let(:params) { { page_token: 'feature', per_page: 2 } }
+      context 'by page_token and per_page' do
+        let(:params) { { page_token: 'feature', per_page: 2 } }
 
-      it 'filters branches' do
-        result = subject
+        it 'filters branches' do
+          result = subject
 
-        expect(result.map(&:name)).to eq(%w(feature_conflict fix))
+          expect(result.map(&:name)).to eq(%w(feature_conflict fix))
+        end
       end
-    end
 
-    context 'by next page_token and per_page' do
-      let(:params) { { page_token: 'fix', per_page: 2 } }
+      context 'by next page_token and per_page' do
+        let(:params) { { page_token: 'fix', per_page: 2 } }
 
-      it 'filters branches' do
-        result = subject
+        it 'filters branches' do
+          result = subject
 
-        expect(result.map(&:name)).to eq(%w(flatten-dir gitattributes))
+          expect(result.map(&:name)).to eq(%w(flatten-dir gitattributes))
+        end
       end
-    end
 
-    context 'by per_page only' do
-      let(:params) { { per_page: 2 } }
+      context 'by per_page only' do
+        let(:params) { { per_page: 2 } }
 
-      it 'filters branches' do
-        result = subject
+        it 'filters branches' do
+          result = subject
 
-        expect(result.map(&:name)).to eq(["'test'", '2-mb-file'])
+          expect(result.map(&:name)).to eq(["'test'", '2-mb-file'])
+        end
       end
-    end
 
-    context 'by page_token only' do
-      let(:params) { { page_token: 'feature' } }
+      context 'by page_token only' do
+        let(:params) { { page_token: 'feature' } }
 
-      it 'returns nothing' do
-        result = subject
+        it 'returns nothing' do
+          result = subject
 
-        expect(result.count).to eq(0)
+          expect(result.count).to eq(0)
+        end
       end
-    end
 
-    context 'pagination and sort' do
-      context 'by per_page' do
-        let(:params) { { sort: 'updated_asc', per_page: 5 } }
+      context 'pagination and sort' do
+        context 'by per_page' do
+          let(:params) { { sort: 'updated_asc', per_page: 5 } }
 
-        it 'filters branches' do
-          result = subject
+          it 'filters branches' do
+            result = subject
 
-          expect(result.map(&:name)).to eq(%w(feature improve/awesome merge-test markdown feature_conflict))
+            expect(result.map(&:name)).to eq(%w(feature improve/awesome merge-test markdown feature_conflict))
+          end
         end
-      end
 
-      context 'by page_token and per_page' do
-        let(:params) { { sort: 'updated_asc', page_token: 'improve/awesome', per_page: 2 } }
+        context 'by page_token and per_page' do
+          let(:params) { { sort: 'updated_asc', page_token: 'improve/awesome', per_page: 2 } }
 
-        it 'filters branches' do
-          result = subject
+          it 'filters branches' do
+            result = subject
 
-          expect(result.map(&:name)).to eq(%w(merge-test markdown))
+            expect(result.map(&:name)).to eq(%w(merge-test markdown))
+          end
         end
       end
-    end
 
-    context 'pagination and names' do
-      let(:params) { { page_token: 'fix', per_page: 2, names: %w[fix csv lfs does-not-exist] } }
+      context 'pagination and names' do
+        let(:params) { { page_token: 'fix', per_page: 2, names: %w[fix csv lfs does-not-exist] } }
 
-      it 'falls back to default execute and ignore paginations' do
-        result = subject
+        it 'falls back to default execute and ignore paginations' do
+          result = subject
 
-        expect(result.count).to eq(3)
-        expect(result.map(&:name)).to eq(%w{csv fix lfs})
+          expect(result.count).to eq(3)
+          expect(result.map(&:name)).to eq(%w{csv fix lfs})
+        end
       end
-    end
 
-    context 'pagination and search' do
-      let(:params) { { page_token: 'feature', per_page: 2, search: '^f' } }
+      context 'pagination and search' do
+        let(:params) { { page_token: 'feature', per_page: 2, search: '^f' } }
 
-      it 'falls back to default execute and ignore paginations' do
-        result = subject
+        it 'falls back to default execute and ignore paginations' do
+          result = subject
 
-        expect(result.map(&:name)).to eq(%w(feature feature_conflict fix flatten-dir))
+          expect(result.map(&:name)).to eq(%w(feature feature_conflict fix flatten-dir))
+        end
       end
     end
   end
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index ef3fca1c23a72f..b4904a00f3e3b6 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -62,11 +62,13 @@ def check_merge_status(json_response)
           end
 
           it 'merge status matches reality on paginated input' do
+            expected_first_branch_name = project.repository.branches_sorted_by('name')[20].name
+
             get api(route, current_user), params: { per_page: 20, page: 2 }
 
             expect(response).to have_gitlab_http_status(:ok)
             expect(json_response.count).to eq 20
-            expect(json_response.first['name']).to eq('conflict-start')
+            expect(json_response.first['name']).to eq(expected_first_branch_name)
 
             check_merge_status(json_response)
           end
@@ -74,11 +76,13 @@ def check_merge_status(json_response)
 
         context 'with gitaly pagination params ' do
           it 'merge status matches reality on paginated input' do
+            expected_first_branch_name = project.repository.branches_sorted_by('name').first.name
+
             get api(route, current_user), params: { per_page: 20, page_token: 'feature' }
 
             expect(response).to have_gitlab_http_status(:ok)
             expect(json_response.count).to eq 20
-            expect(json_response.first['name']).to eq("'test'")
+            expect(json_response.first['name']).to eq(expected_first_branch_name)
 
             check_merge_status(json_response)
           end
@@ -112,22 +116,25 @@ def check_merge_status(json_response)
           end
 
           it 'merge status matches reality on paginated input' do
+            expected_first_branch_name = project.repository.branches_sorted_by('name').drop_while { |b| b.name <= 'feature' }.first.name
+
             get api(route, current_user), params: { per_page: 20, page_token: 'feature' }
 
             expect(response).to have_gitlab_http_status(:ok)
             expect(json_response.count).to eq 20
-            expect(json_response.first['name']).to eq('feature_conflict')
+            expect(json_response.first['name']).to eq(expected_first_branch_name)
 
             check_merge_status(json_response)
           end
         end
 
         context 'with legacy pagination params' do
-          it 'merge status matches reality on paginated input' do
+          it 'ignores legacy pagination params' do
+            expected_first_branch_name = project.repository.branches_sorted_by('name').first.name
             get api(route, current_user), params: { per_page: 20, page: 2 }
 
             expect(response).to have_gitlab_http_status(:ok)
-            expect(json_response.first['name']).to eq("'test'")
+            expect(json_response.first['name']).to eq(expected_first_branch_name)
 
             check_merge_status(json_response)
           end
-- 
GitLab


From 832439c837ace4586ad75633c59b641ab6f34b95 Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Thu, 16 Jul 2020 17:04:50 +0930
Subject: [PATCH 6/7] Rename feature flag name to avoid gitaly prefix

---
 lib/api/branches.rb                | 2 +-
 spec/requests/api/branches_spec.rb | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index 7cada2318d9eaf..20599dba6ac6aa 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -40,7 +40,7 @@ class Branches < Grape::API::Instance
 
         repository = user_project.repository
 
-        if Feature.enabled?(:gitaly_branch_pagination, user_project)
+        if Feature.enabled?(:branch_list_keyset_pagination, user_project)
           branches = BranchesFinder.new(repository, declared_params(include_missing: false)).execute(true)
         else
           branches = BranchesFinder.new(repository, declared_params(include_missing: false)).execute
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index b4904a00f3e3b6..46acd92803f17c 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -17,7 +17,7 @@
   before do
     project.add_maintainer(user)
     project.repository.add_branch(user, 'ends-with.txt', branch_sha)
-    stub_feature_flags(gitaly_branch_pagination: false)
+    stub_feature_flags(branch_list_keyset_pagination: false)
   end
 
   describe "GET /projects/:id/repository/branches" do
@@ -38,7 +38,7 @@ def check_merge_status(json_response)
         expect(project.repository.merged_branch_names(unmerged_branches)).to be_empty
       end
 
-      context 'with gitaly_branch_pagination feature off' do
+      context 'with branch_list_keyset_pagination feature off' do
         context 'with legacy pagination params' do
           it 'returns the repository branches' do
             get api(route, current_user), params: { per_page: 100 }
@@ -89,9 +89,9 @@ def check_merge_status(json_response)
         end
       end
 
-      context 'with gitaly_branch_pagination feature on' do
+      context 'with branch_list_keyset_pagination feature on' do
         before do
-          stub_feature_flags(gitaly_branch_pagination: true)
+          stub_feature_flags(branch_list_keyset_pagination: true)
         end
 
         context 'with gitaly pagination params ' do
-- 
GitLab


From 7d320b3ec4a66f136047b92f046b6ea60ff4454d Mon Sep 17 00:00:00 2001
From: David Kim <dkim@gitlab.com>
Date: Fri, 17 Jul 2020 13:31:51 +0930
Subject: [PATCH 7/7] Change argument to a keyword argument

---
 app/finders/branches_finder.rb       | 2 +-
 lib/api/branches.rb                  | 2 +-
 spec/finders/branches_finder_spec.rb | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/finders/branches_finder.rb b/app/finders/branches_finder.rb
index 4deada3a7de5fe..2eee90a512aff2 100644
--- a/app/finders/branches_finder.rb
+++ b/app/finders/branches_finder.rb
@@ -5,7 +5,7 @@ def initialize(repository, params = {})
     super(repository, params)
   end
 
-  def execute(gitaly_pagination = false)
+  def execute(gitaly_pagination: false)
     if gitaly_pagination && names.blank? && search.blank?
       repository.branches_sorted_by(sort, pagination_params)
     else
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index 20599dba6ac6aa..5e9c2caf8f5dba 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -41,7 +41,7 @@ class Branches < Grape::API::Instance
         repository = user_project.repository
 
         if Feature.enabled?(:branch_list_keyset_pagination, user_project)
-          branches = BranchesFinder.new(repository, declared_params(include_missing: false)).execute(true)
+          branches = BranchesFinder.new(repository, declared_params(include_missing: false)).execute(gitaly_pagination: true)
         else
           branches = BranchesFinder.new(repository, declared_params(include_missing: false)).execute
           branches = paginate(::Kaminari.paginate_array(branches))
diff --git a/spec/finders/branches_finder_spec.rb b/spec/finders/branches_finder_spec.rb
index b61e33235d548c..a62dd3842dbe9b 100644
--- a/spec/finders/branches_finder_spec.rb
+++ b/spec/finders/branches_finder_spec.rb
@@ -173,7 +173,7 @@
     end
 
     context 'with gitaly pagination' do
-      subject { branch_finder.execute(true) }
+      subject { branch_finder.execute(gitaly_pagination: true) }
 
       context 'by page_token and per_page' do
         let(:params) { { page_token: 'feature', per_page: 2 } }
-- 
GitLab