Merge branch '298733-handle-jira-connect-responses' into 'master'

Add handlers for Jira connect error responses

See merge request gitlab-org/gitlab!51738

app/services/jira_connect/sync_service.rb

@@ -31,7 +31,7 @@ module JiraConnect
         jira_response: response&.to_json
       }
 
-      if response && (response['errorMessages'] || response['rejectedBuilds'].present?)
+      if response && response['errorMessages'].present?
         logger.error(message)
       else
         logger.info(message)

lib/atlassian/jira_connect/client.rb

@@ -35,33 +35,44 @@ module Atlassian
       def store_ff_info(project:, feature_flags:, **opts)
         return unless Feature.enabled?(:jira_sync_feature_flags, project)
 
-        items = feature_flags.map { |flag| Serializers::FeatureFlagEntity.represent(flag, opts) }
+        items = feature_flags.map { |flag| ::Atlassian::JiraConnect::Serializers::FeatureFlagEntity.represent(flag, opts) }
         items.reject! { |item| item.issue_keys.empty? }
 
         return if items.empty?
 
-        post('/rest/featureflags/0.1/bulk', {
+        r = post('/rest/featureflags/0.1/bulk', {
           flags: items,
           properties: { projectId: "project-#{project.id}" }
         })
+
+        handle_response(r, 'feature flags') do |data|
+          failed = data['failedFeatureFlags']
+          if failed.present?
+            errors = failed.flat_map do |k, errs|
+              errs.map { |e| "#{k}: #{e['message']}" }
+            end
+            { 'errorMessages' => errors }
+          end
+        end
       end
 
       def store_deploy_info(project:, deployments:, **opts)
         return unless Feature.enabled?(:jira_sync_deployments, project)
 
-        items = deployments.map { |d| Serializers::DeploymentEntity.represent(d, opts) }
+        items = deployments.map { |d| ::Atlassian::JiraConnect::Serializers::DeploymentEntity.represent(d, opts) }
         items.reject! { |d| d.issue_keys.empty? }
 
         return if items.empty?
 
-        post('/rest/deployments/0.1/bulk', { deployments: items })
+        r = post('/rest/deployments/0.1/bulk', { deployments: items })
+        handle_response(r, 'deployments') { |data| errors(data, 'rejectedDeployments') }
       end
 
       def store_build_info(project:, pipelines:, update_sequence_id: nil)
         return unless Feature.enabled?(:jira_sync_builds, project)
 
         builds = pipelines.map do |pipeline|
-          build = Serializers::BuildEntity.represent(
+          build = ::Atlassian::JiraConnect::Serializers::BuildEntity.represent(
             pipeline,
             update_sequence_id: update_sequence_id
           )
@@ -71,7 +82,8 @@ module Atlassian
         end.compact
         return if builds.empty?
 
-        post('/rest/builds/0.1/bulk', { builds: builds })
+        r = post('/rest/builds/0.1/bulk', { builds: builds })
+        handle_response(r, 'builds') { |data| errors(data, 'rejectedBuilds') }
       end
 
       def store_dev_info(project:, commits: nil, branches: nil, merge_requests: nil, update_sequence_id: nil)
@@ -104,6 +116,34 @@ module Atlassian
         { providerMetadata: { product: "GitLab #{Gitlab::VERSION}" } }
       end
 
+      def handle_response(response, name, &block)
+        data = response.parsed_response
+
+        case response.code
+        when 200 then yield data
+        when 400 then { 'errorMessages' => data.map { |e| e['message'] } }
+        when 401 then { 'errorMessages' => ['Invalid JWT'] }
+        when 403 then { 'errorMessages' => ["App does not support #{name}"] }
+        when 413 then { 'errorMessages' => ['Data too large'] + data.map { |e| e['message'] } }
+        when 429 then { 'errorMessages' => ['Rate limit exceeded'] }
+        when 503 then { 'errorMessages' => ['Service unavailable'] }
+        else
+          { 'errorMessages' => ['Unknown error'], 'response' => data }
+        end
+      end
+
+      def errors(data, key)
+        messages = if data[key].present?
+                     data[key].flat_map do |rejection|
+                       rejection['errors'].map { |e| e['message'] }
+                     end
+                   else
+                     []
+                   end
+
+        { 'errorMessages' => messages }
+      end
+
       def user_notes_count(merge_requests)
         return unless merge_requests
 

spec/lib/atlassian/jira_connect/client_spec.rb

@@ -107,6 +107,75 @@ RSpec.describe Atlassian::JiraConnect::Client do
     }
   end
 
+  describe '#handle_response' do
+    let(:errors) { [{ 'message' => 'X' }, { 'message' => 'Y' }] }
+    let(:processed) { subject.send(:handle_response, response, 'foo') { |x| [:data, x] } }
+
+    context 'the response is 200 OK' do
+      let(:response) { double(code: 200, parsed_response: :foo) }
+
+      it 'yields to the block' do
+        expect(processed).to eq [:data, :foo]
+      end
+    end
+
+    context 'the response is 400 bad request' do
+      let(:response) { double(code: 400, parsed_response: errors) }
+
+      it 'extracts the errors messages' do
+        expect(processed).to eq('errorMessages' => %w(X Y))
+      end
+    end
+
+    context 'the response is 401 forbidden' do
+      let(:response) { double(code: 401, parsed_response: nil) }
+
+      it 'reports that our JWT is wrong' do
+        expect(processed).to eq('errorMessages' => ['Invalid JWT'])
+      end
+    end
+
+    context 'the response is 403' do
+      let(:response) { double(code: 403, parsed_response: nil) }
+
+      it 'reports that the App is misconfigured' do
+        expect(processed).to eq('errorMessages' => ['App does not support foo'])
+      end
+    end
+
+    context 'the response is 413' do
+      let(:response) { double(code: 413, parsed_response: errors) }
+
+      it 'extracts the errors messages' do
+        expect(processed).to eq('errorMessages' => ['Data too large', 'X', 'Y'])
+      end
+    end
+
+    context 'the response is 429' do
+      let(:response) { double(code: 429, parsed_response: nil) }
+
+      it 'reports that we exceeded the rate limit' do
+        expect(processed).to eq('errorMessages' => ['Rate limit exceeded'])
+      end
+    end
+
+    context 'the response is 503' do
+      let(:response) { double(code: 503, parsed_response: nil) }
+
+      it 'reports that the service is unavailable' do
+        expect(processed).to eq('errorMessages' => ['Service unavailable'])
+      end
+    end
+
+    context 'the response is anything else' do
+      let(:response) { double(code: 1000, parsed_response: :something) }
+
+      it 'reports that this was unanticipated' do
+        expect(processed).to eq('errorMessages' => ['Unknown error'], 'response' => :something)
+      end
+    end
+  end
+
   describe '#store_deploy_info' do
     let_it_be(:environment) { create(:environment, name: 'DEV', project: project) }
     let_it_be(:deployments) do
@@ -126,10 +195,20 @@ RSpec.describe Atlassian::JiraConnect::Client do
       ->(text) { matcher.matches?(text) }
     end
 
+    let(:rejections) { [] }
+    let(:response_body) do
+      {
+        acceptedDeployments: [],
+        rejectedDeployments: rejections,
+        unknownIssueKeys: []
+      }.to_json
+    end
+
     before do
       path = '/rest/deployments/0.1/bulk'
       stub_full_request('https://gitlab-test.atlassian.net' + path, method: :post)
         .with(body: body, headers: expected_headers(path))
+        .to_return(body: response_body, headers: { 'Content-Type': 'application/json' })
     end
 
     it "calls the API with auth headers" do
@@ -137,7 +216,7 @@ RSpec.describe Atlassian::JiraConnect::Client do
     end
 
     it 'only sends information about relevant MRs' do
-      expect(subject).to receive(:post).with('/rest/deployments/0.1/bulk', { deployments: have_attributes(size: 6) })
+      expect(subject).to receive(:post).with('/rest/deployments/0.1/bulk', { deployments: have_attributes(size: 6) }).and_call_original
 
       subject.send(:store_deploy_info, project: project, deployments: deployments)
     end
@@ -148,6 +227,18 @@ RSpec.describe Atlassian::JiraConnect::Client do
       subject.send(:store_deploy_info, project: project, deployments: deployments.take(1))
     end
 
+    context 'there are errors' do
+      let(:rejections) do
+        [{ errors: [{ message: 'X' }, { message: 'Y' }] }, { errors: [{ message: 'Z' }] }]
+      end
+
+      it 'reports the errors' do
+        response = subject.send(:store_deploy_info, project: project, deployments: deployments)
+
+        expect(response['errorMessages']).to eq(%w(X Y Z))
+      end
+    end
+
     it 'does not call the API if the feature flag is not enabled' do
       stub_feature_flags(jira_sync_deployments: false)
 
@@ -159,7 +250,7 @@ RSpec.describe Atlassian::JiraConnect::Client do
     it 'does call the API if the feature flag enabled for the project' do
       stub_feature_flags(jira_sync_deployments: project)
 
-      expect(subject).to receive(:post).with('/rest/deployments/0.1/bulk', { deployments: Array })
+      expect(subject).to receive(:post).with('/rest/deployments/0.1/bulk', { deployments: Array }).and_call_original
 
       subject.send(:store_deploy_info, project: project, deployments: deployments)
     end
@@ -178,12 +269,22 @@ RSpec.describe Atlassian::JiraConnect::Client do
       ->(text) { matcher.matches?(text) }
     end
 
+    let(:failures) { {} }
+    let(:response_body) do
+      {
+        acceptedFeatureFlags: [],
+        failedFeatureFlags: failures,
+        unknownIssueKeys: []
+      }.to_json
+    end
+
     before do
       feature_flags.first.update!(description: 'RELEVANT-123')
       feature_flags.second.update!(description: 'RELEVANT-123')
       path = '/rest/featureflags/0.1/bulk'
       stub_full_request('https://gitlab-test.atlassian.net' + path, method: :post)
         .with(body: body, headers: expected_headers(path))
+        .to_return(body: response_body, headers: { 'Content-Type': 'application/json' })
     end
 
     it "calls the API with auth headers" do
@@ -193,7 +294,7 @@ RSpec.describe Atlassian::JiraConnect::Client do
     it 'only sends information about relevant MRs' do
       expect(subject).to receive(:post).with('/rest/featureflags/0.1/bulk', {
         flags: have_attributes(size: 2), properties: Hash
-      })
+      }).and_call_original
 
       subject.send(:store_ff_info, project: project, feature_flags: feature_flags)
     end
@@ -204,6 +305,21 @@ RSpec.describe Atlassian::JiraConnect::Client do
       subject.send(:store_ff_info, project: project, feature_flags: [feature_flags.last])
     end
 
+    context 'there are errors' do
+      let(:failures) do
+        {
+          a: [{ message: 'X' }, { message: 'Y' }],
+          b: [{ message: 'Z' }]
+        }
+      end
+
+      it 'reports the errors' do
+        response = subject.send(:store_ff_info, project: project, feature_flags: feature_flags)
+
+        expect(response['errorMessages']).to eq(['a: X', 'a: Y', 'b: Z'])
+      end
+    end
+
     it 'does not call the API if the feature flag is not enabled' do
       stub_feature_flags(jira_sync_feature_flags: false)
 
@@ -217,7 +333,7 @@ RSpec.describe Atlassian::JiraConnect::Client do
 
       expect(subject).to receive(:post).with('/rest/featureflags/0.1/bulk', {
         flags: Array, properties: Hash
-      })
+      }).and_call_original
 
       subject.send(:store_ff_info, project: project, feature_flags: feature_flags)
     end
@@ -234,10 +350,20 @@ RSpec.describe Atlassian::JiraConnect::Client do
       ->(text) { matcher.matches?(text) }
     end
 
+    let(:failures) { [] }
+    let(:response_body) do
+      {
+        acceptedBuilds: [],
+        rejectedBuilds: failures,
+        unknownIssueKeys: []
+      }.to_json
+    end
+
     before do
       path = '/rest/builds/0.1/bulk'
       stub_full_request('https://gitlab-test.atlassian.net' + path, method: :post)
         .with(body: body, headers: expected_headers(path))
+        .to_return(body: response_body, headers: { 'Content-Type': 'application/json' })
     end
 
     it "calls the API with auth headers" do
@@ -245,7 +371,9 @@ RSpec.describe Atlassian::JiraConnect::Client do
     end
 
     it 'only sends information about relevant MRs' do
-      expect(subject).to receive(:post).with('/rest/builds/0.1/bulk', { builds: have_attributes(size: 6) })
+      expect(subject).to receive(:post)
+        .with('/rest/builds/0.1/bulk', { builds: have_attributes(size: 6) })
+        .and_call_original
 
       subject.send(:store_build_info, project: project, pipelines: pipelines)
     end
@@ -267,11 +395,25 @@ RSpec.describe Atlassian::JiraConnect::Client do
     it 'does call the API if the feature flag enabled for the project' do
       stub_feature_flags(jira_sync_builds: project)
 
-      expect(subject).to receive(:post).with('/rest/builds/0.1/bulk', { builds: Array })
+      expect(subject).to receive(:post)
+        .with('/rest/builds/0.1/bulk', { builds: Array })
+        .and_call_original
 
       subject.send(:store_build_info, project: project, pipelines: pipelines)
     end
 
+    context 'there are errors' do
+      let(:failures) do
+        [{ errors: [{ message: 'X' }, { message: 'Y' }] }, { errors: [{ message: 'Z' }] }]
+      end
+
+      it 'reports the errors' do
+        response = subject.send(:store_build_info, project: project, pipelines: pipelines)
+
+        expect(response['errorMessages']).to eq(%w(X Y Z))
+      end
+    end
+
     it 'avoids N+1 database queries' do
       pending 'https://gitlab.com/gitlab-org/gitlab/-/issues/292818'
 

spec/services/jira_connect/sync_service_spec.rb

@@ -45,11 +45,11 @@ RSpec.describe JiraConnect::SyncService do
       it 'logs the response as an error' do
         expect_next(client).to store_info([
           { 'errorMessages' => ['some error message'] },
-          { 'rejectedBuilds' => ['x'] }
+          { 'errorMessages' => ['x'] }
         ])
 
         expect_log(:error, { 'errorMessages' => ['some error message'] })
-        expect_log(:error, { 'rejectedBuilds' => ['x'] })
+        expect_log(:error, { 'errorMessages' => ['x'] })
 
         subject
       end