Limit diverging commit counts requests

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).

Limit diverging commit counts requests
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).
dae0dec0 · Stan Hu · 3de934cb · dae0dec0 · dae0dec0 · dae0dec0
--- 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 @@ def destroy_all_merged

  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]))

--- a/changelogs/unreleased/sh-limit-diverging-commit-counts.yml
+++ b/changelogs/unreleased/sh-limit-diverging-commit-counts.yml
+---
+title: Limit diverging commit counts requests
+merge_request: 32497
+author:
+type: performance
--- a/spec/controllers/projects/branches_controller_spec.rb
+++ b/spec/controllers/projects/branches_controller_spec.rb
@@ -578,7 +578,9 @@ def destroy_all_merged
  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 @@ def destroy_all_merged
            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