diff options
author | Peter Leitzen <pleitzen@gitlab.com> | 2019-04-07 10:51:36 +0300 |
---|---|---|
committer | Michael Kozono <mkozono@gmail.com> | 2019-04-07 10:51:36 +0300 |
commit | bbb17ea1ea619664b362a9c984da45b940c412a2 (patch) | |
tree | 617df6d7bd4186c7f1eec0374e975375323d20b9 | |
parent | ae91b3219aa0b5de20e3452126384341acef75c6 (diff) |
Handle possible HTTP exception for Sentry client
Prior this commit exceptions raised during a HTTP request
weren't caught by the Sentry client and were passed to the user.
In addition the Sentry client tried to catch a non-existent error
`Sentry::Client::SentryError`.
Now, the Sentry client catches all possible errors coming from
a HTTP request.
5 files changed, 93 insertions, 20 deletions
diff --git a/app/services/error_tracking/list_projects_service.rb b/app/services/error_tracking/list_projects_service.rb index 4e92353a13c..8d08f0cda94 100644 --- a/app/services/error_tracking/list_projects_service.rb +++ b/app/services/error_tracking/list_projects_service.rb @@ -15,8 +15,8 @@ module ErrorTracking result = setting.list_sentry_projects rescue Sentry::Client::Error => e return error(e.message, :bad_request) - rescue Sentry::Client::SentryError => e - return error(e.message, :unprocessable_entity) + rescue Sentry::Client::MissingKeysError => e + return error(e.message, :internal_server_error) end success(projects: result[:projects]) diff --git a/changelogs/unreleased/60149-nameerror-uninitialized-constant-sentry-client-sentryerror.yml b/changelogs/unreleased/60149-nameerror-uninitialized-constant-sentry-client-sentryerror.yml new file mode 100644 index 00000000000..8c3a47cf62c --- /dev/null +++ b/changelogs/unreleased/60149-nameerror-uninitialized-constant-sentry-client-sentryerror.yml @@ -0,0 +1,5 @@ +--- +title: Handle possible HTTP exception for Sentry client +merge_request: 27080 +author: +type: fixed diff --git a/lib/sentry/client.rb b/lib/sentry/client.rb index bb1aa2a7a10..4022e8ff946 100644 --- a/lib/sentry/client.rb +++ b/lib/sentry/client.rb @@ -47,9 +47,11 @@ module Sentry end def http_get(url, params = {}) - resp = Gitlab::HTTP.get(url, **request_params.merge(params)) + response = handle_request_exceptions do + Gitlab::HTTP.get(url, **request_params.merge(params)) + end - handle_response(resp) + handle_response(response) end def get_issues(issue_status:, limit:) @@ -63,14 +65,36 @@ module Sentry http_get(projects_api_url) end + def handle_request_exceptions + yield + rescue HTTParty::Error => e + Gitlab::Sentry.track_acceptable_exception(e) + raise_error 'Error when connecting to Sentry' + rescue Net::OpenTimeout + raise_error 'Connection to Sentry timed out' + rescue SocketError + raise_error 'Received SocketError when trying to connect to Sentry' + rescue OpenSSL::SSL::SSLError + raise_error 'Sentry returned invalid SSL data' + rescue Errno::ECONNREFUSED + raise_error 'Connection refused' + rescue => e + Gitlab::Sentry.track_acceptable_exception(e) + raise_error "Sentry request failed due to #{e.class}" + end + def handle_response(response) unless response.code == 200 - raise Client::Error, "Sentry response status code: #{response.code}" + raise_error "Sentry response status code: #{response.code}" end response end + def raise_error(message) + raise Client::Error, message + end + def projects_api_url projects_url = URI(@url) projects_url.path = '/api/0/projects/' diff --git a/spec/lib/sentry/client_spec.rb b/spec/lib/sentry/client_spec.rb index 3333f8307ae..cb14204b99a 100644 --- a/spec/lib/sentry/client_spec.rb +++ b/spec/lib/sentry/client_spec.rb @@ -61,13 +61,37 @@ describe Sentry::Client do end end + shared_examples 'maps exceptions' do + exceptions = { + HTTParty::Error => 'Error when connecting to Sentry', + Net::OpenTimeout => 'Connection to Sentry timed out', + SocketError => 'Received SocketError when trying to connect to Sentry', + OpenSSL::SSL::SSLError => 'Sentry returned invalid SSL data', + Errno::ECONNREFUSED => 'Connection refused', + StandardError => 'Sentry request failed due to StandardError' + } + + exceptions.each do |exception, message| + context "#{exception}" do + before do + stub_request(:get, sentry_request_url).to_raise(exception) + end + + it do + expect { subject } + .to raise_exception(Sentry::Client::Error, message) + end + end + end + end + describe '#list_issues' do let(:issue_status) { 'unresolved' } let(:limit) { 20 } - let(:sentry_api_response) { issues_sample_response } + let(:sentry_request_url) { sentry_url + '/issues/?limit=20&query=is:unresolved' } - let!(:sentry_api_request) { stub_sentry_request(sentry_url + '/issues/?limit=20&query=is:unresolved', body: sentry_api_response) } + let!(:sentry_api_request) { stub_sentry_request(sentry_request_url, body: sentry_api_response) } subject { client.list_issues(issue_status: issue_status, limit: limit) } @@ -121,16 +145,14 @@ describe Sentry::Client do # Sentry API returns 404 if there are extra slashes in the URL! context 'extra slashes in URL' do let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0/projects//sentry-org/sentry-project/' } - let(:client) { described_class.new(sentry_url, token) } - let!(:valid_req_stub) do - stub_sentry_request( - 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project/' \ + let(:sentry_request_url) do + 'https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project/' \ 'issues/?limit=20&query=is:unresolved' - ) end it 'removes extra slashes in api url' do + expect(client.url).to eq(sentry_url) expect(Gitlab::HTTP).to receive(:get).with( URI('https://sentrytest.gitlab.com/api/0/projects/sentry-org/sentry-project/issues/'), anything @@ -138,7 +160,7 @@ describe Sentry::Client do subject - expect(valid_req_stub).to have_been_requested + expect(sentry_api_request).to have_been_requested end end @@ -169,6 +191,8 @@ describe Sentry::Client do expect { subject }.to raise_error(Sentry::Client::MissingKeysError, 'Sentry API response is missing keys. key not found: "id"') end end + + it_behaves_like 'maps exceptions' end describe '#list_projects' do @@ -260,12 +284,18 @@ describe Sentry::Client do expect(valid_req_stub).to have_been_requested end end + + context 'when exception is raised' do + let(:sentry_request_url) { sentry_list_projects_url } + + it_behaves_like 'maps exceptions' + end end private def stub_sentry_request(url, body: {}, status: 200, headers: {}) - WebMock.stub_request(:get, url) + stub_request(:get, url) .to_return( status: status, headers: { 'Content-Type' => 'application/json' }.merge(headers), diff --git a/spec/services/error_tracking/list_projects_service_spec.rb b/spec/services/error_tracking/list_projects_service_spec.rb index a92d3376f7b..730fccc599e 100644 --- a/spec/services/error_tracking/list_projects_service_spec.rb +++ b/spec/services/error_tracking/list_projects_service_spec.rb @@ -51,14 +51,28 @@ describe ErrorTracking::ListProjectsService do end context 'sentry client raises exception' do - before do - expect(error_tracking_setting).to receive(:list_sentry_projects) - .and_raise(Sentry::Client::Error, 'Sentry response status code: 500') + context 'Sentry::Client::Error' do + before do + expect(error_tracking_setting).to receive(:list_sentry_projects) + .and_raise(Sentry::Client::Error, 'Sentry response status code: 500') + end + + it 'returns error response' do + expect(result[:message]).to eq('Sentry response status code: 500') + expect(result[:http_status]).to eq(:bad_request) + end end - it 'returns error response' do - expect(result[:message]).to eq('Sentry response status code: 500') - expect(result[:http_status]).to eq(:bad_request) + context 'Sentry::Client::MissingKeysError' do + before do + expect(error_tracking_setting).to receive(:list_sentry_projects) + .and_raise(Sentry::Client::MissingKeysError, 'Sentry API response is missing keys. key not found: "id"') + end + + it 'returns error response' do + expect(result[:message]).to eq('Sentry API response is missing keys. key not found: "id"') + expect(result[:http_status]).to eq(:internal_server_error) + end end end |