From dcc67ac1fdf21f3e9b301ba91acadf8adadc1cf9 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 29 Feb 2016 13:54:33 +0100 Subject: [PATCH 1/5] 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 --- lib/api/commit_statuses.rb | 10 +- spec/requests/api/commit_status_spec.rb | 116 +++++++++++++++--------- 2 files changed, 80 insertions(+), 46 deletions(-) diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 9422d438d21..f98fdd4e159 100644 --- a/lib/api/commit_statuses.rb +++ b/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 - sha = params[:sha] - ci_commit = user_project.ci_commit(sha) - not_found! 'Commit' unless ci_commit + authorize!(:read_commit_status, user_project) + + ci_commit = user_project.ci_commit(params[:sha]) + not_found!('Commit') unless user_project.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? diff --git a/spec/requests/api/commit_status_spec.rb b/spec/requests/api/commit_status_spec.rb index 89b554622ef..b03fc045a94 100644 --- a/spec/requests/api/commit_status_spec.rb +++ b/spec/requests/api/commit_status_spec.rb @@ -2,63 +2,95 @@ 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) } describe "GET /projects/:id/repository/commits/:sha/statuses" do - it_behaves_like 'a paginated resources' do - 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("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", reporter) } + end + + context "reporter user" do + let(:statuses_id) { json_response.map { |status| status['id'] } } + + let!(:status1) do + create(:commit_status, commit: ci_commit, status: 'running') + end + + let!(:status2) do + create(:commit_status, commit: ci_commit, name: 'coverage', status: 'pending') + end + + let!(:status3) do + create(:commit_status, commit: ci_commit, name: 'coverage', ref: 'develop', + status: 'running', allow_failure: true) + end + + let!(:status4) do + create(:commit_status, commit: ci_commit, name: 'coverage', status: 'success') + end + + let!(:status5) do + create(:commit_status, commit: ci_commit, ref: 'develop', status: 'success') + end + + let!(:status6) do + 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) + + 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 + + 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 end - context "reporter user" do - let(:statuses_id) { json_response.map { |status| status['id'] } } - + context 'ci commit does not exist' do 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) - - 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 - 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) - + it 'returns empty array' 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 - - 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) + expect(json_response).to be_empty end end From 433b4581ff9e20d7263d0819f18ba662290f3c33 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 29 Feb 2016 14:20:11 +0100 Subject: [PATCH 2/5] Improve commit status API specs (refactoring) --- spec/requests/api/commit_status_spec.rb | 117 +++++++++++++++--------- 1 file changed, 76 insertions(+), 41 deletions(-) diff --git a/spec/requests/api/commit_status_spec.rb b/spec/requests/api/commit_status_spec.rb index b03fc045a94..10730bfc0da 100644 --- a/spec/requests/api/commit_status_spec.rb +++ b/spec/requests/api/commit_status_spec.rb @@ -11,6 +11,8 @@ describe API::CommitStatus, api: true do let(:developer) { create_user(ProjectMember::DEVELOPER) } describe "GET /projects/:id/repository/commits/:sha/statuses" do + let(:get_url) { "/projects/#{project.id}/repository/commits/#{commit.id}/statuses" } + context 'ci commit exists' do let!(:ci_commit) { project.ensure_ci_commit(commit.id) } @@ -46,46 +48,56 @@ describe API::CommitStatus, api: true do 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) + context 'latest commit statuses' do + before { get api(get_url, reporter) } - 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]) + 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 - 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) + context 'all commit statuses' do + before { get api(get_url, reporter), all: 1 } - 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) + 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 - 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) + context 'latest commit statuses for specific ref' do + before { get api(get_url, reporter), ref: 'develop' } - expect(json_response).to be_an Array - expect(statuses_id).to contain_exactly(status3.id, status5.id) + 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 - 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) + context 'latest commit statues for specific name' do + before { get api(get_url, reporter), name: 'coverage' } - expect(json_response).to be_an Array - expect(statuses_id).to contain_exactly(status3.id, status4.id) + 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(status3.id, status4.id) + end end end end context 'ci commit does not exist' do - before do - get api("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", reporter) - end + before { get api(get_url, reporter) } it 'returns empty array' do expect(response.status).to eq 200 @@ -95,15 +107,17 @@ describe API::CommitStatus, api: true do 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 @@ -113,9 +127,10 @@ describe API::CommitStatus, api: true do let(:post_url) { "/projects/#{project.id}/statuses/#{commit.id}" } context 'developer user' do - context 'should create commit status' do - it 'with only required parameters' do - post api(post_url, developer), state: 'success' + context 'only required parameters' do + before { 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') @@ -124,9 +139,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 - post api(post_url, developer), state: 'success', context: 'coverage', ref: 'develop', target_url: 'url', description: 'test' + context 'with all optional parameters' do + 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') @@ -137,41 +160,53 @@ describe API::CommitStatus, api: true do end end - context 'should not create commit status' do - it 'with invalid state' do - post api(post_url, developer), state: 'invalid' + context 'invalid status' do + before { 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 - post api(post_url, developer) + context 'request without state' do + 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 + before do post api("/projects/#{project.id}/statuses/invalid_sha", developer), state: 'running' + end + + 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 From 8c66d739c12beedb89f617faa0aab568eea37705 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 29 Feb 2016 14:30:01 +0100 Subject: [PATCH 3/5] Add Changelog entry for commit status API changes --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 9e897644af0..61c9eeaf355 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,7 @@ v 8.6.0 (unreleased) - Improve the formatting for the user page bio (Connor Shea) - 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 From 85a461e096dae7436993eba73b205fd916249e46 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 1 Mar 2016 11:11:50 +0100 Subject: [PATCH 4/5] Check if commit exists first in commit status API --- lib/api/commit_statuses.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index f98fdd4e159..8e74e177ea0 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -20,8 +20,8 @@ module API get ':id/repository/commits/:sha/statuses' do authorize!(:read_commit_status, user_project) - ci_commit = user_project.ci_commit(params[:sha]) not_found!('Commit') unless user_project.commit(params[:sha]) + ci_commit = user_project.ci_commit(params[:sha]) return [] unless ci_commit statuses = ci_commit.statuses From 278f4423d1bbec596d3556582439e09b2376367b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 1 Mar 2016 12:33:25 +0100 Subject: [PATCH 5/5] Improve commit status API specs --- spec/requests/api/commit_status_spec.rb | 49 ++++++++++--------------- 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/spec/requests/api/commit_status_spec.rb b/spec/requests/api/commit_status_spec.rb index 10730bfc0da..8017ed97d88 100644 --- a/spec/requests/api/commit_status_spec.rb +++ b/spec/requests/api/commit_status_spec.rb @@ -9,44 +9,32 @@ describe API::CommitStatus, api: true do 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 - let(:get_url) { "/projects/#{project.id}/repository/commits/#{commit.id}/statuses" } + let(:get_url) { "/projects/#{project.id}/repository/commits/#{sha}/statuses" } 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("/projects/#{project.id}/repository/commits/#{commit.id}/statuses", reporter) } + let(:request) { get api(get_url, reporter) } end context "reporter user" do let(:statuses_id) { json_response.map { |status| status['id'] } } - let!(:status1) do - create(:commit_status, commit: ci_commit, status: 'running') + def create_status(opts = {}) + create(:commit_status, { commit: ci_commit }.merge(opts)) end - let!(:status2) do - create(:commit_status, commit: ci_commit, name: 'coverage', status: 'pending') - end - - let!(:status3) do - create(:commit_status, commit: ci_commit, name: 'coverage', ref: 'develop', - status: 'running', allow_failure: true) - end - - let!(:status4) do - create(:commit_status, commit: ci_commit, name: 'coverage', status: 'success') - end - - let!(:status5) do - create(:commit_status, commit: ci_commit, ref: 'develop', status: 'success') - end - - let!(:status6) do - create(:commit_status, commit: ci_commit, status: 'success') - 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) } @@ -68,7 +56,9 @@ describe API::CommitStatus, api: true 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) + expect(statuses_id).to contain_exactly(status1.id, status2.id, + status3.id, status4.id, + status5.id, status6.id) end end @@ -90,7 +80,7 @@ describe API::CommitStatus, api: true do expect(response.status).to eq(200) expect(json_response).to be_an Array - expect(statuses_id).to contain_exactly(status3.id, status4.id) + expect(statuses_id).to contain_exactly(status4.id, status5.id) end end end @@ -124,7 +114,7 @@ describe API::CommitStatus, api: true do 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 'only required parameters' do @@ -177,9 +167,8 @@ describe API::CommitStatus, api: true do end context 'invalid commit' do - before do - post api("/projects/#{project.id}/statuses/invalid_sha", developer), state: 'running' - end + let(:sha) { 'invalid_sha' } + before { post api(post_url, developer), state: 'running' } it 'returns not found error' do expect(response.status).to eq(404)