Automatic merge of gitlab-org/gitlab-ce master

app/assets/javascripts/error_tracking/store/actions.js

@@ -20,7 +20,7 @@ export function startPolling({ commit, dispatch }, endpoint) {
       commit(types.SET_LOADING, false);
       dispatch('stopPolling');
     },
-    errorCallback: response => {
+    errorCallback: ({ response }) => {
       let errorMessage = '';
       if (response && response.data && response.data.message) {
         errorMessage = response.data.message;

app/models/error_tracking/project_error_tracking_setting.rb

@@ -5,6 +5,9 @@ module ErrorTracking
     include Gitlab::Utils::StrongMemoize
     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{
       \A
         (?<prefix>/api/0/projects/+)
@@ -90,7 +93,9 @@ module ErrorTracking
         { issues: sentry_client.list_issues(**opts.symbolize_keys) }
       end
     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
 
     # http://HOST/api/0/projects/ORG/PROJECT

app/services/error_tracking/list_issues_service.rb

@@ -18,7 +18,7 @@ module ErrorTracking
       end
 
       if result[:error].present?
-        return error(result[:error], :bad_request)
+        return error(result[:error], http_status_from_error_type(result[:error_type]))
       end
 
       success(issues: result[:issues])
@@ -30,6 +30,15 @@ module ErrorTracking
 
     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
       project.error_tracking_setting
     end

changelogs/unreleased/58971-sentry-api-keyerror.yml

+---
+title: Handle missing keys in sentry api response
+merge_request: 26264
+author:
+type: fixed

lib/sentry/client.rb

@@ -3,7 +3,7 @@
 module Sentry
   class Client
     Error = Class.new(StandardError)
-    SentryError = Class.new(StandardError)
+    MissingKeysError = Class.new(StandardError)
 
     attr_accessor :url, :token
 
@@ -14,18 +14,29 @@ module Sentry
 
     def list_issues(issue_status:, limit:)
       issues = get_issues(issue_status: issue_status, limit: limit)
-      map_to_errors(issues)
+
+      handle_mapping_exceptions do
+        map_to_errors(issues)
+      end
     end
 
     def list_projects
       projects = get_projects
-      map_to_projects(projects)
-    rescue KeyError => e
-      raise Client::SentryError, "Sentry API response is missing keys. #{e.message}"
+
+      handle_mapping_exceptions do
+        map_to_projects(projects)
+      end
     end
 
     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
       {
         headers: {
@@ -94,7 +105,6 @@ module Sentry
 
     def map_to_error(issue)
       id = issue.fetch('id')
-      project = issue.fetch('project')
 
       count = issue.fetch('count', nil)
 
@@ -117,9 +127,9 @@ module Sentry
         short_id: issue.fetch('shortId', nil),
         status: issue.fetch('status', nil),
         frequency: frequency,
-        project_id: project.fetch('id'),
-        project_name: project.fetch('name', nil),
-        project_slug: project.fetch('slug', nil)
+        project_id: issue.dig('project', 'id'),
+        project_name: issue.dig('project', 'name'),
+        project_slug: issue.dig('project', 'slug')
       )
     end
 
@@ -127,12 +137,12 @@ module Sentry
       organization = project.fetch('organization')
 
       Gitlab::ErrorTracking::Project.new(
-        id: project.fetch('id'),
+        id: project.fetch('id', nil),
         name: project.fetch('name'),
         slug: project.fetch('slug'),
         status: project.dig('status'),
         organization_name: organization.fetch('name'),
-        organization_id: organization.fetch('id'),
+        organization_id: organization.fetch('id', nil),
         organization_slug: organization.fetch('slug')
       )
     end

spec/lib/sentry/client_spec.rb

@@ -65,7 +65,9 @@ describe Sentry::Client do
     let(:issue_status) { 'unresolved' }
     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) }
 
@@ -74,6 +76,14 @@ describe Sentry::Client do
     it_behaves_like 'has correct return type', Gitlab::ErrorTracking::Error
     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
       using RSpec::Parameterized::TableSyntax
 
@@ -96,14 +106,10 @@ describe Sentry::Client do
       end
 
       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
 
-      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
+      it_behaves_like 'has correct external_url'
     end
 
     context 'redirects' do
@@ -135,12 +141,42 @@ describe Sentry::Client do
         expect(valid_req_stub).to have_been_requested
       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
 
   describe '#list_projects' do
     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 }
 
@@ -149,14 +185,31 @@ describe Sentry::Client do
     it_behaves_like 'has correct return type', Gitlab::ErrorTracking::Project
     it_behaves_like 'has correct length', 2
 
-    context 'keys missing in API response' do
-      it 'raises exception' do
-        projects_sample_response[0].delete(:slug)
+    context 'essential keys missing in API response' do
+      let(:sentry_api_response) do
+        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
+
+      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
@@ -173,7 +226,11 @@ describe Sentry::Client do
       end
 
       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
 

spec/models/error_tracking/project_error_tracking_setting_spec.rb

@@ -167,7 +167,7 @@ describe ErrorTracking::ProjectErrorTrackingSetting do
       end
     end
 
-    context 'when sentry client raises exception' do
+    context 'when sentry client raises Sentry::Client::Error' do
       let(:sentry_client) { spy(:sentry_client) }
 
       before do
@@ -179,7 +179,31 @@ describe ErrorTracking::ProjectErrorTrackingSetting do
       end
 
       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(sentry_client).to have_received(:list_issues)
       end

spec/services/error_tracking/list_issues_service_spec.rb

@@ -53,7 +53,10 @@ describe ErrorTracking::ListIssuesService do
         before do
           allow(error_tracking_setting)
             .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
 
         it 'returns the error' do
@@ -64,6 +67,25 @@ describe ErrorTracking::ListIssuesService do
           )
         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
 
     context 'with unauthorized user' do