Commit 8749b019 authored by Sean McGivern's avatar Sean McGivern

Merge branch '58971-sentry-api-keyerror' into 'master'

Handle missing keys in sentry api response

Closes #58971

See merge request gitlab-org/gitlab-ce!26264
parents bf48b071 c558d72b
...@@ -20,7 +20,7 @@ export function startPolling({ commit, dispatch }, endpoint) { ...@@ -20,7 +20,7 @@ export function startPolling({ commit, dispatch }, endpoint) {
commit(types.SET_LOADING, false); commit(types.SET_LOADING, false);
dispatch('stopPolling'); dispatch('stopPolling');
}, },
errorCallback: response => { errorCallback: ({ response }) => {
let errorMessage = ''; let errorMessage = '';
if (response && response.data && response.data.message) { if (response && response.data && response.data.message) {
errorMessage = response.data.message; errorMessage = response.data.message;
......
...@@ -5,6 +5,9 @@ module ErrorTracking ...@@ -5,6 +5,9 @@ module ErrorTracking
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
include ReactiveCaching include ReactiveCaching
SENTRY_API_ERROR_TYPE_MISSING_KEYS = 'missing_keys_in_sentry_response'
SENTRY_API_ERROR_TYPE_NON_20X_RESPONSE = 'non_20x_response_from_sentry'
API_URL_PATH_REGEXP = %r{ API_URL_PATH_REGEXP = %r{
\A \A
(?<prefix>/api/0/projects/+) (?<prefix>/api/0/projects/+)
...@@ -90,7 +93,9 @@ module ErrorTracking ...@@ -90,7 +93,9 @@ module ErrorTracking
{ issues: sentry_client.list_issues(**opts.symbolize_keys) } { issues: sentry_client.list_issues(**opts.symbolize_keys) }
end end
rescue Sentry::Client::Error => e rescue Sentry::Client::Error => e
{ error: e.message } { error: e.message, error_type: SENTRY_API_ERROR_TYPE_NON_20X_RESPONSE }
rescue Sentry::Client::MissingKeysError => e
{ error: e.message, error_type: SENTRY_API_ERROR_TYPE_MISSING_KEYS }
end end
# http://HOST/api/0/projects/ORG/PROJECT # http://HOST/api/0/projects/ORG/PROJECT
......
...@@ -18,7 +18,7 @@ module ErrorTracking ...@@ -18,7 +18,7 @@ module ErrorTracking
end end
if result[:error].present? if result[:error].present?
return error(result[:error], :bad_request) return error(result[:error], http_status_from_error_type(result[:error_type]))
end end
success(issues: result[:issues]) success(issues: result[:issues])
...@@ -30,6 +30,15 @@ module ErrorTracking ...@@ -30,6 +30,15 @@ module ErrorTracking
private private
def http_status_from_error_type(error_type)
case error_type
when ErrorTracking::ProjectErrorTrackingSetting::SENTRY_API_ERROR_TYPE_MISSING_KEYS
:internal_server_error
else
:bad_request
end
end
def project_error_tracking_setting def project_error_tracking_setting
project.error_tracking_setting project.error_tracking_setting
end end
......
---
title: Handle missing keys in sentry api response
merge_request: 26264
author:
type: fixed
...@@ -3,7 +3,7 @@ ...@@ -3,7 +3,7 @@
module Sentry module Sentry
class Client class Client
Error = Class.new(StandardError) Error = Class.new(StandardError)
SentryError = Class.new(StandardError) MissingKeysError = Class.new(StandardError)
attr_accessor :url, :token attr_accessor :url, :token
...@@ -14,18 +14,29 @@ module Sentry ...@@ -14,18 +14,29 @@ module Sentry
def list_issues(issue_status:, limit:) def list_issues(issue_status:, limit:)
issues = get_issues(issue_status: issue_status, limit: limit) issues = get_issues(issue_status: issue_status, limit: limit)
handle_mapping_exceptions do
map_to_errors(issues) map_to_errors(issues)
end end
end
def list_projects def list_projects
projects = get_projects projects = get_projects
handle_mapping_exceptions do
map_to_projects(projects) map_to_projects(projects)
rescue KeyError => e end
raise Client::SentryError, "Sentry API response is missing keys. #{e.message}"
end end
private private
def handle_mapping_exceptions(&block)
yield
rescue KeyError => e
Gitlab::Sentry.track_acceptable_exception(e)
raise Client::MissingKeysError, "Sentry API response is missing keys. #{e.message}"
end
def request_params def request_params
{ {
headers: { headers: {
...@@ -94,7 +105,6 @@ module Sentry ...@@ -94,7 +105,6 @@ module Sentry
def map_to_error(issue) def map_to_error(issue)
id = issue.fetch('id') id = issue.fetch('id')
project = issue.fetch('project')
count = issue.fetch('count', nil) count = issue.fetch('count', nil)
...@@ -117,9 +127,9 @@ module Sentry ...@@ -117,9 +127,9 @@ module Sentry
short_id: issue.fetch('shortId', nil), short_id: issue.fetch('shortId', nil),
status: issue.fetch('status', nil), status: issue.fetch('status', nil),
frequency: frequency, frequency: frequency,
project_id: project.fetch('id'), project_id: issue.dig('project', 'id'),
project_name: project.fetch('name', nil), project_name: issue.dig('project', 'name'),
project_slug: project.fetch('slug', nil) project_slug: issue.dig('project', 'slug')
) )
end end
...@@ -127,12 +137,12 @@ module Sentry ...@@ -127,12 +137,12 @@ module Sentry
organization = project.fetch('organization') organization = project.fetch('organization')
Gitlab::ErrorTracking::Project.new( Gitlab::ErrorTracking::Project.new(
id: project.fetch('id'), id: project.fetch('id', nil),
name: project.fetch('name'), name: project.fetch('name'),
slug: project.fetch('slug'), slug: project.fetch('slug'),
status: project.dig('status'), status: project.dig('status'),
organization_name: organization.fetch('name'), organization_name: organization.fetch('name'),
organization_id: organization.fetch('id'), organization_id: organization.fetch('id', nil),
organization_slug: organization.fetch('slug') organization_slug: organization.fetch('slug')
) )
end end
......
...@@ -65,7 +65,9 @@ describe Sentry::Client do ...@@ -65,7 +65,9 @@ describe Sentry::Client do
let(:issue_status) { 'unresolved' } let(:issue_status) { 'unresolved' }
let(:limit) { 20 } let(:limit) { 20 }
let!(:sentry_api_request) { stub_sentry_request(sentry_url + '/issues/?limit=20&query=is:unresolved', body: issues_sample_response) } let(:sentry_api_response) { issues_sample_response }
let!(:sentry_api_request) { stub_sentry_request(sentry_url + '/issues/?limit=20&query=is:unresolved', body: sentry_api_response) }
subject { client.list_issues(issue_status: issue_status, limit: limit) } subject { client.list_issues(issue_status: issue_status, limit: limit) }
...@@ -74,6 +76,14 @@ describe Sentry::Client do ...@@ -74,6 +76,14 @@ describe Sentry::Client do
it_behaves_like 'has correct return type', Gitlab::ErrorTracking::Error it_behaves_like 'has correct return type', Gitlab::ErrorTracking::Error
it_behaves_like 'has correct length', 1 it_behaves_like 'has correct length', 1
shared_examples 'has correct external_url' do
context 'external_url' do
it 'is constructed correctly' do
expect(subject[0].external_url).to eq('https://sentrytest.gitlab.com/sentry-org/sentry-project/issues/11')
end
end
end
context 'error object created from sentry response' do context 'error object created from sentry response' do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
...@@ -96,14 +106,10 @@ describe Sentry::Client do ...@@ -96,14 +106,10 @@ describe Sentry::Client do
end end
with_them do with_them do
it { expect(subject[0].public_send(error_object)).to eq(issues_sample_response[0].dig(*sentry_response)) } it { expect(subject[0].public_send(error_object)).to eq(sentry_api_response[0].dig(*sentry_response)) }
end end
context 'external_url' do it_behaves_like 'has correct external_url'
it 'is constructed correctly' do
expect(subject[0].external_url).to eq('https://sentrytest.gitlab.com/sentry-org/sentry-project/issues/11')
end
end
end end
context 'redirects' do context 'redirects' do
...@@ -135,12 +141,42 @@ describe Sentry::Client do ...@@ -135,12 +141,42 @@ describe Sentry::Client do
expect(valid_req_stub).to have_been_requested expect(valid_req_stub).to have_been_requested
end end
end end
context 'Older sentry versions where keys are not present' do
let(:sentry_api_response) do
issues_sample_response[0...1].map do |issue|
issue[:project].delete(:id)
issue
end
end
it_behaves_like 'calls sentry api'
it_behaves_like 'has correct return type', Gitlab::ErrorTracking::Error
it_behaves_like 'has correct length', 1
it_behaves_like 'has correct external_url'
end
context 'essential keys missing in API response' do
let(:sentry_api_response) do
issues_sample_response[0...1].map do |issue|
issue.except(:id)
end
end
it 'raises exception' do
expect { subject }.to raise_error(Sentry::Client::MissingKeysError, 'Sentry API response is missing keys. key not found: "id"')
end
end
end end
describe '#list_projects' do describe '#list_projects' do
let(:sentry_list_projects_url) { 'https://sentrytest.gitlab.com/api/0/projects/' } let(:sentry_list_projects_url) { 'https://sentrytest.gitlab.com/api/0/projects/' }
let!(:sentry_api_request) { stub_sentry_request(sentry_list_projects_url, body: projects_sample_response) } let(:sentry_api_response) { projects_sample_response }
let!(:sentry_api_request) { stub_sentry_request(sentry_list_projects_url, body: sentry_api_response) }
subject { client.list_projects } subject { client.list_projects }
...@@ -149,16 +185,33 @@ describe Sentry::Client do ...@@ -149,16 +185,33 @@ describe Sentry::Client do
it_behaves_like 'has correct return type', Gitlab::ErrorTracking::Project it_behaves_like 'has correct return type', Gitlab::ErrorTracking::Project
it_behaves_like 'has correct length', 2 it_behaves_like 'has correct length', 2
context 'keys missing in API response' do context 'essential keys missing in API response' do
it 'raises exception' do let(:sentry_api_response) do
projects_sample_response[0].delete(:slug) projects_sample_response[0...1].map do |project|
project.except(:slug)
end
end
stub_sentry_request(sentry_list_projects_url, body: projects_sample_response) it 'raises exception' do
expect { subject }.to raise_error(Sentry::Client::MissingKeysError, 'Sentry API response is missing keys. key not found: "slug"')
end
end
expect { subject }.to raise_error(Sentry::Client::SentryError, 'Sentry API response is missing keys. key not found: "slug"') context 'optional keys missing in sentry response' do
let(:sentry_api_response) do
projects_sample_response[0...1].map do |project|
project[:organization].delete(:id)
project.delete(:id)
project.except(:status)
end end
end end
it_behaves_like 'calls sentry api'
it_behaves_like 'has correct return type', Gitlab::ErrorTracking::Project
it_behaves_like 'has correct length', 1
end
context 'error object created from sentry response' do context 'error object created from sentry response' do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
...@@ -173,7 +226,11 @@ describe Sentry::Client do ...@@ -173,7 +226,11 @@ describe Sentry::Client do
end end
with_them do with_them do
it { expect(subject[0].public_send(sentry_project_object)).to eq(projects_sample_response[0].dig(*sentry_response)) } it do
expect(subject[0].public_send(sentry_project_object)).to(
eq(sentry_api_response[0].dig(*sentry_response))
)
end
end end
end end
......
...@@ -167,7 +167,7 @@ describe ErrorTracking::ProjectErrorTrackingSetting do ...@@ -167,7 +167,7 @@ describe ErrorTracking::ProjectErrorTrackingSetting do
end end
end end
context 'when sentry client raises exception' do context 'when sentry client raises Sentry::Client::Error' do
let(:sentry_client) { spy(:sentry_client) } let(:sentry_client) { spy(:sentry_client) }
before do before do
...@@ -179,7 +179,31 @@ describe ErrorTracking::ProjectErrorTrackingSetting do ...@@ -179,7 +179,31 @@ describe ErrorTracking::ProjectErrorTrackingSetting do
end end
it 'returns error' do it 'returns error' do
expect(result).to eq(error: 'error message') expect(result).to eq(
error: 'error message',
error_type: ErrorTracking::ProjectErrorTrackingSetting::SENTRY_API_ERROR_TYPE_NON_20X_RESPONSE
)
expect(subject).to have_received(:sentry_client)
expect(sentry_client).to have_received(:list_issues)
end
end
context 'when sentry client raises Sentry::Client::MissingKeysError' do
let(:sentry_client) { spy(:sentry_client) }
before do
synchronous_reactive_cache(subject)
allow(subject).to receive(:sentry_client).and_return(sentry_client)
allow(sentry_client).to receive(:list_issues).with(opts)
.and_raise(Sentry::Client::MissingKeysError, 'Sentry API response is missing keys. key not found: "id"')
end
it 'returns error' do
expect(result).to eq(
error: 'Sentry API response is missing keys. key not found: "id"',
error_type: ErrorTracking::ProjectErrorTrackingSetting::SENTRY_API_ERROR_TYPE_MISSING_KEYS
)
expect(subject).to have_received(:sentry_client) expect(subject).to have_received(:sentry_client)
expect(sentry_client).to have_received(:list_issues) expect(sentry_client).to have_received(:list_issues)
end end
......
...@@ -53,7 +53,10 @@ describe ErrorTracking::ListIssuesService do ...@@ -53,7 +53,10 @@ describe ErrorTracking::ListIssuesService do
before do before do
allow(error_tracking_setting) allow(error_tracking_setting)
.to receive(:list_sentry_issues) .to receive(:list_sentry_issues)
.and_return(error: 'Sentry response status code: 401') .and_return(
error: 'Sentry response status code: 401',
error_type: ErrorTracking::ProjectErrorTrackingSetting::SENTRY_API_ERROR_TYPE_NON_20X_RESPONSE
)
end end
it 'returns the error' do it 'returns the error' do
...@@ -64,6 +67,25 @@ describe ErrorTracking::ListIssuesService do ...@@ -64,6 +67,25 @@ describe ErrorTracking::ListIssuesService do
) )
end end
end end
context 'when list_sentry_issues returns error with http_status' do
before do
allow(error_tracking_setting)
.to receive(:list_sentry_issues)
.and_return(
error: 'Sentry API response is missing keys. key not found: "id"',
error_type: ErrorTracking::ProjectErrorTrackingSetting::SENTRY_API_ERROR_TYPE_MISSING_KEYS
)
end
it 'returns the error with correct http_status' do
expect(result).to eq(
status: :error,
http_status: :internal_server_error,
message: 'Sentry API response is missing keys. key not found: "id"'
)
end
end
end end
context 'with unauthorized user' do context 'with unauthorized user' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment