Limit diverging commit counts requestssh-limit-diverging-commit-counts

In https://gitlab.com/gitlab-org/gitlab-ce/issues/66200, we saw that
many clients accidentally request diverging counts for all branches only
because there are no stale/active branches in the project. This has been
fixed on the frontend in
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/32496.

To prevent this endpoint from calling Gitaly too many times, we require
that branch names be specified if the total number of branches exceeds
the limit (20).

3 files changed, 69 insertions, 2 deletions

diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index e7bdb4b2042..7ad60bbab8a 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -4,6 +4,8 @@ class Projects::BranchesController < Projects::ApplicationController
   include ActionView::Helpers::SanitizeHelper
   include SortingHelper
 
+  DIVERGING_COUNT_BRANCH_LIMIT = 20
+
   # Authorize
   before_action :require_non_empty_project, except: :create
   before_action :authorize_download_code!
@@ -11,6 +13,7 @@ class Projects::BranchesController < Projects::ApplicationController
 
   # Support legacy URLs
   before_action :redirect_for_legacy_index_sort_or_search, only: [:index]
+  before_action :limit_diverging_commit_counts!, only: [:diverging_commit_counts]
 
   def index
     respond_to do |format|
@@ -125,6 +128,19 @@ class Projects::BranchesController < Projects::ApplicationController
 
   private
 
+  # It can be expensive to calculate the diverging counts for each
+  # branch. Normally the frontend should be specifying a set of branch
+  # names, but due to frontend bugs this doesn't always happen. To
+  # prevent excessive I/O, we require that a list of names be specified.
+  def limit_diverging_commit_counts!
+    return unless Feature.enabled?(:limit_diverging_commit_counts, enabled: true)
+    # If we don't have many branches in the repository, then go ahead.
+    return false if project.repository.branch_count <= DIVERGING_COUNT_BRANCH_LIMIT
+    return if params[:names].present? && Array(params[:names]).length <= DIVERGING_COUNT_BRANCH_LIMIT
+
+    render json: { error: "Specify at least one and at most #{DIVERGING_COUNT_BRANCH_LIMIT} branch names" }, status: :unprocessable_entity
+  end
+
   def ref
     if params[:ref]
       ref_escaped = strip_tags(sanitize(params[:ref]))
diff --git a/changelogs/unreleased/sh-limit-diverging-commit-counts.yml b/changelogs/unreleased/sh-limit-diverging-commit-counts.yml
new file mode 100644
index 00000000000..3aa47bd1fbe
--- /dev/null
+++ b/changelogs/unreleased/sh-limit-diverging-commit-counts.yml
@@ -0,0 +1,5 @@
+---
+title: Limit diverging commit counts requests
+merge_request: 32497
+author:
+type: performance
diff --git a/spec/controllers/projects/branches_controller_spec.rb b/spec/controllers/projects/branches_controller_spec.rb
index f5bcea4a097..61bf68b881c 100644
--- a/spec/controllers/projects/branches_controller_spec.rb
+++ b/spec/controllers/projects/branches_controller_spec.rb
@@ -578,7 +578,9 @@ describe Projects::BranchesController do
   describe 'GET diverging_commit_counts' do
     before do
       sign_in(user)
+    end
 
+    it 'returns the commit counts behind and ahead of default branch' do
       get :diverging_commit_counts,
           format: :json,
           params: {
@@ -586,14 +588,58 @@ describe Projects::BranchesController do
             project_id: project,
             names: ['fix', 'add-pdf-file', 'branch-merged']
           }
-    end
 
-    it 'returns the commit counts behind and ahead of default branch' do
+      expect(response).to have_gitlab_http_status(200)
       expect(json_response).to eq(
         "fix" => { "behind" => 29, "ahead" => 2 },
         "branch-merged" => { "behind" => 1, "ahead" => 0 },
         "add-pdf-file" => { "behind" => 0, "ahead" => 3 }
       )
     end
+
+    it 'returns the commits counts with no names provided' do
+      allow_any_instance_of(Repository).to receive(:branch_count).and_return(described_class::DIVERGING_COUNT_BRANCH_LIMIT)
+
+      get :diverging_commit_counts,
+          format: :json,
+          params: {
+            namespace_id: project.namespace,
+            project_id: project
+          }
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response.count).to be > 1
+    end
+
+    describe 'with many branches' do
+      before do
+        allow_any_instance_of(Repository).to receive(:branch_count).and_return(described_class::DIVERGING_COUNT_BRANCH_LIMIT + 1)
+      end
+
+      it 'returns 422 if no names are specified' do
+        get :diverging_commit_counts,
+            format: :json,
+            params: {
+              namespace_id: project.namespace,
+              project_id: project
+            }
+
+        expect(response).to have_gitlab_http_status(422)
+        expect(json_response['error']).to eq("Specify at least one and at most #{described_class::DIVERGING_COUNT_BRANCH_LIMIT} branch names")
+      end
+
+      it 'returns the list of counts' do
+        get :diverging_commit_counts,
+            format: :json,
+            params: {
+              namespace_id: project.namespace,
+              project_id: project,
+              names: ['fix', 'add-pdf-file', 'branch-merged']
+            }
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response.count).to be > 1
+      end
+    end
   end
 end