Merge branch '37032-get-project-branch-invalid-name-message' into 'master'

Get Project Branch API shows an helpful error message on invalid refname

Closes #37032

See merge request gitlab-org/gitlab-ce!14884

changelogs/unreleased/37032-get-project-branch-invalid-name-message.yml

@@ -0,0 +1,5 @@
+---
+title: Get Project Branch API shows an helpful error message on invalid refname
+merge_request: 14884
+author: Jacopo Beschi @jacopo-beschi
+type: added

lib/api/branches.rb

@@ -8,6 +8,16 @@ module API
 
     before { authorize! :download_code, user_project }
 
+    helpers do
+      def find_branch!(branch_name)
+        begin
+          user_project.repository.find_branch(branch_name) || not_found!('Branch')
+        rescue Gitlab::Git::CommandError
+          render_api_error!('The branch refname is invalid', 400)
+        end
+      end
+    end
+
     params do
       requires :id, type: String, desc: 'The ID of a project'
     end
@@ -38,8 +48,7 @@ module API
           user_project.repository.branch_exists?(params[:branch]) ? status(204) : status(404)
         end
         get do
-          branch = user_project.repository.find_branch(params[:branch])
-          not_found!('Branch') unless branch
+          branch = find_branch!(params[:branch])
 
           present branch, with: Entities::Branch, project: user_project
         end
@@ -60,8 +69,7 @@ module API
       put ':id/repository/branches/:branch/protect', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         authorize_admin_project
 
-        branch = user_project.repository.find_branch(params[:branch])
-        not_found!('Branch') unless branch
+        branch = find_branch!(params[:branch])
 
         protected_branch = user_project.protected_branches.find_by(name: branch.name)
 
@@ -96,8 +104,7 @@ module API
       put ':id/repository/branches/:branch/unprotect', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         authorize_admin_project
 
-        branch = user_project.repository.find_branch(params[:branch])
-        not_found!("Branch") unless branch
+        branch = find_branch!(params[:branch])
         protected_branch = user_project.protected_branches.find_by(name: branch.name)
         protected_branch&.destroy
 
@@ -133,8 +140,7 @@ module API
       delete ':id/repository/branches/:branch', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         authorize_push_project
 
-        branch = user_project.repository.find_branch(params[:branch])
-        not_found!('Branch') unless branch
+        branch = find_branch!(params[:branch])
 
         commit = user_project.repository.commit(branch.dereferenced_target)
 

spec/requests/api/branches_spec.rb

@@ -110,6 +110,15 @@ describe API::Branches do
         end
       end
 
+      context 'when the branch refname is invalid' do
+        let(:branch_name) { 'branch*' }
+        let(:message) { 'The branch refname is invalid' }
+
+        it_behaves_like '400 response' do
+          let(:request) { get api(route, current_user) }
+        end
+      end
+
       context 'when repository is disabled' do
         include_context 'disabled repository'
 
@@ -234,6 +243,15 @@ describe API::Branches do
         end
       end
 
+      context 'when the branch refname is invalid' do
+        let(:branch_name) { 'branch*' }
+        let(:message) { 'The branch refname is invalid' }
+
+        it_behaves_like '400 response' do
+          let(:request) { put api(route, current_user) }
+        end
+      end
+
       context 'when repository is disabled' do
         include_context 'disabled repository'
 
@@ -359,6 +377,15 @@ describe API::Branches do
         end
       end
 
+      context 'when the branch refname is invalid' do
+        let(:branch_name) { 'branch*' }
+        let(:message) { 'The branch refname is invalid' }
+
+        it_behaves_like '400 response' do
+          let(:request) { put api(route, current_user) }
+        end
+      end
+
       context 'when repository is disabled' do
         include_context 'disabled repository'
 
@@ -520,6 +547,15 @@ describe API::Branches do
       expect(response).to have_gitlab_http_status(404)
     end
 
+    context 'when the branch refname is invalid' do
+      let(:branch_name) { 'branch*' }
+      let(:message) { 'The branch refname is invalid' }
+
+      it_behaves_like '400 response' do
+        let(:request) { delete api("/projects/#{project.id}/repository/branches/#{branch_name}", user) }
+      end
+    end
+
     it_behaves_like '412 response' do
       let(:request) { api("/projects/#{project.id}/repository/branches/#{branch_name}", user) }
     end

spec/support/shared_examples/requests/api/status_shared_examples.rb

@@ -3,6 +3,8 @@
 # Requires an API request:
 #   let(:request) { get api("/projects/#{project.id}/repository/branches", user) }
 shared_examples_for '400 response' do
+  let(:message) { nil }
+
   before do
     # Fires the request
     request
@@ -10,6 +12,10 @@ shared_examples_for '400 response' do
 
   it 'returns 400' do
     expect(response).to have_gitlab_http_status(400)
+
+    if message.present?
+      expect(json_response['message']).to eq(message)
+    end
   end
 end
 
@@ -26,6 +32,7 @@ end
 
 shared_examples_for '404 response' do
   let(:message) { nil }
+
   before do
     # Fires the request
     request