Merge branch 'fix/commit-status-api-improvement' into 'master'

Return empty array when commit has no statuses in API

This makes API endpoint for Commit Statuses return empty array instead
of 404 when commit exists, but has no statuses.

Closes #3080

See merge request !3010

CHANGELOG

@@ -6,6 +6,7 @@ v 8.6.0 (unreleased)
   - Fix issue when pushing to projects ending in .wiki
   - Fix avatar stretching by providing a cropping feature (Johann Pardanaud)
   - Strip leading and trailing spaces in URL validator (evuez)
+  - Return empty array instead of 404 when commit has no statuses in commit status API
   - Update documentation to reflect Guest role not being enforced on internal projects
 
 v 8.5.2

lib/api/commit_statuses.rb

@@ -18,10 +18,12 @@ module API
       # Examples:
       #   GET /projects/:id/repository/commits/:sha/statuses
       get ':id/repository/commits/:sha/statuses' do
-        authorize! :read_commit_status, user_project
+        authorize!(:read_commit_status, user_project)
-        sha = params[:sha]
+
-        ci_commit = user_project.ci_commit(sha)
+        not_found!('Commit') unless user_project.commit(params[:sha])
-        not_found! 'Commit' unless ci_commit
+        ci_commit = user_project.ci_commit(params[:sha])
+        return [] unless ci_commit
+
         statuses = ci_commit.statuses
         statuses = statuses.latest unless parse_boolean(params[:all])
         statuses = statuses.where(ref: params[:ref]) if params[:ref].present?

spec/requests/api/commit_status_spec.rb

@@ -2,88 +2,125 @@ require 'spec_helper'
 
 describe API::CommitStatus, api: true do
   include ApiHelpers
+
   let!(:project) { create(:project) }
   let(:commit) { project.repository.commit }
-  let!(:ci_commit) { project.ensure_ci_commit(commit.id) }
   let(:commit_status) { create(:commit_status, commit: ci_commit) }
   let(:guest) { create_user(ProjectMember::GUEST) }
   let(:reporter) { create_user(ProjectMember::REPORTER) }
   let(:developer) { create_user(ProjectMember::DEVELOPER) }
+  let(:sha) { commit.id }
+
 
   describe "GET /projects/:id/repository/commits/:sha/statuses" do
-    it_behaves_like 'a paginated resources' do
+    let(:get_url) { "/projects/#{project.id}/repository/commits/#{sha}/statuses" }
-      let(:request) { get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", reporter) }
+
+    context 'ci commit exists' do
+      let!(:ci_commit) { project.ensure_ci_commit(commit.id) }
+
+      it_behaves_like 'a paginated resources' do
+        let(:request) { get api(get_url, reporter) }
+      end
+
+      context "reporter user" do
+        let(:statuses_id) { json_response.map { |status| status['id'] } }
+
+        def create_status(opts = {})
+          create(:commit_status, { commit: ci_commit }.merge(opts))
+        end
+
+        let!(:status1) { create_status(status: 'running') }
+        let!(:status2) { create_status(name: 'coverage', status: 'pending') }
+        let!(:status3) { create_status(ref: 'develop', status: 'running', allow_failure: true) }
+        let!(:status4) { create_status(name: 'coverage', status: 'success') }
+        let!(:status5) { create_status(name: 'coverage', ref: 'develop', status: 'success') }
+        let!(:status6) { create_status(status: 'success') }
+
+        context 'latest commit statuses' do
+          before { get api(get_url, reporter) }
+
+          it 'returns latest commit statuses' do
+            expect(response.status).to eq(200)
+
+            expect(json_response).to be_an Array
+            expect(statuses_id).to contain_exactly(status3.id, status4.id, status5.id, status6.id)
+            json_response.sort_by!{ |status| status['id'] }
+            expect(json_response.map{ |status| status['allow_failure'] }).to eq([true, false, false, false])
+          end
+        end
+
+        context 'all commit statuses' do
+          before { get api(get_url, reporter), all: 1 }
+
+          it 'returns all commit statuses' do
+            expect(response.status).to eq(200)
+
+            expect(json_response).to be_an Array
+            expect(statuses_id).to contain_exactly(status1.id, status2.id,
+                                                   status3.id, status4.id,
+                                                   status5.id, status6.id)
+          end
+        end
+
+        context 'latest commit statuses for specific ref' do
+          before { get api(get_url, reporter), ref: 'develop' }
+
+          it 'returns latest commit statuses for specific ref' do
+            expect(response.status).to eq(200)
+
+            expect(json_response).to be_an Array
+            expect(statuses_id).to contain_exactly(status3.id, status5.id)
+          end
+        end
+
+        context 'latest commit statues for specific name' do
+          before { get api(get_url, reporter), name: 'coverage' }
+
+          it 'return latest commit statuses for specific name' do
+            expect(response.status).to eq(200)
+
+            expect(json_response).to be_an Array
+            expect(statuses_id).to contain_exactly(status4.id, status5.id)
+          end
+        end
+      end
     end
 
-    context "reporter user" do
+    context 'ci commit does not exist' do
-      let(:statuses_id) { json_response.map { |status| status['id'] } }
+      before { get api(get_url, reporter) }
-
-      before do
-        @status1 = create(:commit_status, commit: ci_commit, status: 'running')
-        @status2 = create(:commit_status, commit: ci_commit, name: 'coverage', status: 'pending')
-        @status3 = create(:commit_status, commit: ci_commit, name: 'coverage', ref: 'develop', status: 'running', allow_failure: true)
-        @status4 = create(:commit_status, commit: ci_commit, name: 'coverage', status: 'success')
-        @status5 = create(:commit_status, commit: ci_commit, ref: 'develop', status: 'success')
-        @status6 = create(:commit_status, commit: ci_commit, status: 'success')
-      end
-
-      it "should return latest commit statuses" do
-        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", reporter)
-        expect(response.status).to eq(200)
 
+      it 'returns empty array' do
+        expect(response.status).to eq 200
         expect(json_response).to be_an Array
-        expect(statuses_id).to contain_exactly(@status3.id, @status4.id, @status5.id, @status6.id)
+        expect(json_response).to be_empty
-        json_response.sort_by!{ |status| status['id'] }
-        expect(json_response.map{ |status| status['allow_failure'] }).to eq([true, false, false, false])
-      end
-
-      it "should return all commit statuses" do
-        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?all=1", reporter)
-        expect(response.status).to eq(200)
-
-        expect(json_response).to be_an Array
-        expect(statuses_id).to contain_exactly(@status1.id, @status2.id, @status3.id, @status4.id, @status5.id, @status6.id)
-      end
-
-      it "should return latest commit statuses for specific ref" do
-        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?ref=develop", reporter)
-        expect(response.status).to eq(200)
-
-        expect(json_response).to be_an Array
-        expect(statuses_id).to contain_exactly(@status3.id, @status5.id)
-      end
-
-      it "should return latest commit statuses for specific name" do
-        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses?name=coverage", reporter)
-        expect(response.status).to eq(200)
-
-        expect(json_response).to be_an Array
-        expect(statuses_id).to contain_exactly(@status3.id, @status4.id)
       end
     end
 
     context "guest user" do
+      before { get api(get_url, guest) }
+
       it "should not return project commits" do
-        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", guest)
         expect(response.status).to eq(403)
       end
     end
 
     context "unauthorized user" do
+      before { get api(get_url) }
+
       it "should not return project commits" do
-        get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses")
         expect(response.status).to eq(401)
       end
     end
   end
 
   describe 'POST /projects/:id/statuses/:sha' do
-    let(:post_url) { "/projects/#{project.id}/statuses/#{commit.id}" }
+    let(:post_url) { "/projects/#{project.id}/statuses/#{sha}" }
 
     context 'developer user' do
-      context 'should create commit status' do
+      context 'only required parameters' do
-        it 'with only required parameters' do
+        before { post api(post_url, developer), state: 'success' }
-          post api(post_url, developer), state: 'success'
+
+        it 'creates commit status' do
           expect(response.status).to eq(201)
           expect(json_response['sha']).to eq(commit.id)
           expect(json_response['status']).to eq('success')
@@ -92,9 +129,17 @@ describe API::CommitStatus, api: true do
           expect(json_response['target_url']).to be_nil
           expect(json_response['description']).to be_nil
         end
+      end
 
-        it 'with all optional parameters' do
+      context 'with all optional parameters' do
-          post api(post_url, developer), state: 'success', context: 'coverage', ref: 'develop', target_url: 'url', description: 'test'
+        before do
+          optional_params = { state: 'success', context: 'coverage',
+                              ref: 'develop', target_url: 'url', description: 'test' }
+
+          post api(post_url, developer), optional_params
+        end
+
+        it 'creates commit status' do
           expect(response.status).to eq(201)
           expect(json_response['sha']).to eq(commit.id)
           expect(json_response['status']).to eq('success')
@@ -105,41 +150,52 @@ describe API::CommitStatus, api: true do
         end
       end
 
-      context 'should not create commit status' do
+      context 'invalid status' do
-        it 'with invalid state' do
+        before { post api(post_url, developer), state: 'invalid' }
-          post api(post_url, developer), state: 'invalid'
+
+        it 'does not create commit status' do
           expect(response.status).to eq(400)
         end
+      end
 
-        it 'without state' do
+      context 'request without state' do
-          post api(post_url, developer)
+        before { post api(post_url, developer) }
+
+        it 'does not create commit status' do
           expect(response.status).to eq(400)
         end
+      end
 
-        it 'invalid commit' do
+      context 'invalid commit' do
-          post api("/projects/#{project.id}/statuses/invalid_sha", developer), state: 'running'
+        let(:sha) { 'invalid_sha' }
+        before { post api(post_url, developer), state: 'running' }
+
+        it 'returns not found error' do
           expect(response.status).to eq(404)
         end
       end
     end
 
     context 'reporter user' do
+      before { post api(post_url, reporter) }
+
       it 'should not create commit status' do
-        post api(post_url, reporter)
         expect(response.status).to eq(403)
       end
     end
 
     context 'guest user' do
+      before { post api(post_url, guest) }
+
       it 'should not create commit status' do
-        post api(post_url, guest)
         expect(response.status).to eq(403)
       end
     end
 
     context 'unauthorized user' do
+      before { post api(post_url) }
+
       it 'should not create commit status' do
-        post api(post_url)
         expect(response.status).to eq(401)
       end
     end