Move creation of external cross-references into background worker

Calling external APIs can cause delays and timeouts, so to avoid this
when creating cross references for external issues we move this
processing into a dedicated background worker.

Changelog: changed

app/services/system_notes/issuables_service.rb

@@ -176,7 +176,13 @@ module SystemNotes
       body = cross_reference_note_content(gfm_reference)
 
       if noteable.is_a?(ExternalIssue)
-        noteable.project.external_issue_tracker.create_cross_reference_note(noteable, mentioner, author)
+        Integrations::CreateExternalCrossReferenceWorker.perform_async(
+          noteable.project_id,
+          noteable.id,
+          mentioner.class.name,
+          mentioner.id,
+          author.id
+        )
       else
         track_cross_reference_action
         create_note(NoteSummary.new(noteable, noteable.project, author, body, action: 'cross_reference'))

app/workers/all_queues.yml

@@ -2231,6 +2231,15 @@
   :weight: 2
   :idempotent: true
   :tags: []
+- :name: integrations_create_external_cross_reference
+  :worker_name: Integrations::CreateExternalCrossReferenceWorker
+  :feature_category: :integrations
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: invalid_gpg_signature_update
   :worker_name: InvalidGpgSignatureUpdateWorker
   :feature_category: :source_code_management

app/workers/integrations/create_external_cross_reference_worker.rb

+# frozen_string_literal: true
+
+module Integrations
+  class CreateExternalCrossReferenceWorker
+    include ApplicationWorker
+
+    data_consistency :delayed
+
+    feature_category :integrations
+    urgency :low
+    idempotent!
+    deduplicate :until_executed, including_scheduled: true
+    loggable_arguments 2
+
+    def perform(project_id, external_issue_id, mentionable_type, mentionable_id, author_id)
+      project = Project.find_by_id(project_id) || return
+      author = User.find_by_id(author_id) || return
+      mentionable = find_mentionable(mentionable_type, mentionable_id, project) || return
+      external_issue = ExternalIssue.new(external_issue_id, project)
+
+      project.external_issue_tracker.create_cross_reference_note(
+        external_issue,
+        mentionable,
+        author
+      )
+    end
+
+    private
+
+    def find_mentionable(mentionable_type, mentionable_id, project)
+      mentionable_class = mentionable_type.safe_constantize
+
+      # Passing an invalid mentionable_class is a developer error, so we don't want to retry the job
+      # but still track the exception on production, and raise it in development.
+      unless mentionable_class && mentionable_class < Mentionable
+        Gitlab::ErrorTracking.track_and_raise_for_dev_exception(ArgumentError.new("Unexpected class '#{mentionable_type}' is not a Mentionable"))
+        return
+      end
+
+      if mentionable_type == 'Commit'
+        project.commit(mentionable_id)
+      else
+        mentionable_class.find_by_id(mentionable_id)
+      end
+    end
+  end
+end

config/sidekiq_queues.yml

@@ -201,6 +201,8 @@
   - 1
 - - incident_management_pending_escalations_alert_create
   - 1
+- - integrations_create_external_cross_reference
+  - 1
 - - invalid_gpg_signature_update
   - 2
 - - irker

spec/models/integrations/jira_spec.rb

@@ -857,10 +857,14 @@ RSpec.describe Integrations::Jira do
     let_it_be(:user) { build_stubbed(:user) }
 
     let(:jira_issue) { ExternalIssue.new('JIRA-123', project) }
+    let(:success_message) { 'SUCCESS: Successfully posted to http://jira.example.com.' }
+    let(:favicon_path) { "http://localhost/assets/#{find_asset('favicon.png').digest_path}" }
 
     subject { jira_integration.create_cross_reference_note(jira_issue, resource, user) }
 
-    shared_examples 'creates a comment on Jira' do
+    shared_examples 'handles cross-references' do
+      let(:resource_name) { jira_integration.send(:noteable_name, resource) }
+      let(:resource_url) { jira_integration.send(:build_entity_url, resource_name, resource.to_param) }
       let(:issue_url) { "#{url}/rest/api/2/issue/JIRA-123" }
       let(:comment_url) { "#{issue_url}/comment" }
       let(:remote_link_url) { "#{issue_url}/remotelink" }
@@ -872,12 +876,65 @@ RSpec.describe Integrations::Jira do
         stub_request(:post, remote_link_url).with(basic_auth: [username, password])
       end
 
-      it 'creates a comment on Jira' do
-        subject
+      context 'when enabled' do
+        before do
+          allow(jira_integration).to receive(:can_cross_reference?) { true }
+        end
 
-        expect(WebMock).to have_requested(:post, comment_url).with(
-          body: /mentioned this issue.*on branch \[master/
-        ).once
+        it 'creates a comment and remote link' do
+          expect(subject).to eq(success_message)
+          expect(WebMock).to have_requested(:post, comment_url).with(body: comment_body).once
+          expect(WebMock).to have_requested(:post, remote_link_url).with(
+            body: hash_including(
+              GlobalID: 'GitLab',
+              relationship: 'mentioned on',
+              object: {
+                url: resource_url,
+                title: "#{resource.model_name.human} - #{resource.title}",
+                icon: { title: 'GitLab', url16x16: favicon_path },
+                status: { resolved: false }
+              }
+            )
+          ).once
+        end
+
+        context 'when comment already exists' do
+          before do
+            allow(jira_integration).to receive(:comment_exists?) { true }
+          end
+
+          it 'does not create a comment or remote link' do
+            expect(subject).to be_nil
+            expect(WebMock).not_to have_requested(:post, comment_url)
+            expect(WebMock).not_to have_requested(:post, remote_link_url)
+          end
+        end
+
+        context 'when remote link already exists' do
+          let(:link) { double(object: { 'url' => resource_url }) }
+
+          before do
+            allow(jira_integration).to receive(:find_remote_link).and_return(link)
+          end
+
+          it 'updates the remote link but does not create a comment' do
+            expect(link).to receive(:save!)
+            expect(subject).to eq(success_message)
+            expect(WebMock).not_to have_requested(:post, comment_url)
+          end
+        end
+      end
+
+      context 'when disabled' do
+        before do
+          allow(jira_integration).to receive(:can_cross_reference?) { false }
+        end
+
+        it 'does not create a comment or remote link' do
+          expect(subject).to eq("Events for #{resource_name.pluralize.humanize(capitalize: false)} are disabled.")
+          expect(WebMock).not_to have_requested(:post, comment_url)
+          expect(WebMock).not_to have_requested(:post, remote_link_url)
+        end
       end
 
       context 'with jira_use_first_ref_by_oid feature flag disabled' do
@@ -885,12 +942,10 @@ RSpec.describe Integrations::Jira do
           stub_feature_flags(jira_use_first_ref_by_oid: false)
         end
 
-        it 'creates a comment on Jira' do
-          subject
-
-          expect(WebMock).to have_requested(:post, comment_url).with(
-            body: /mentioned this issue.*on branch \[master/
-          ).once
+        it 'creates a comment and remote link on Jira' do
+          expect(subject).to eq(success_message)
+          expect(WebMock).to have_requested(:post, comment_url).with(body: comment_body).once
+          expect(WebMock).to have_requested(:post, remote_link_url).once
         end
       end
 
@@ -903,39 +958,38 @@ RSpec.describe Integrations::Jira do
       end
     end
 
-    context 'when resource is a commit' do
-      let(:resource) { project.commit('master') }
-
-      context 'when disabled' do
-        before do
-          allow_next_instance_of(described_class) do |instance|
-            allow(instance).to receive(:commit_events) { false }
-          end
-        end
-
-        it { is_expected.to eq('Events for commits are disabled.') }
+    context 'for commits' do
+      it_behaves_like 'handles cross-references' do
+        let(:resource) { project.commit('master') }
+        let(:comment_body) { /mentioned this issue in \[a commit\|.* on branch \[master\|/ }
       end
+    end
 
-      context 'when enabled' do
-        it_behaves_like 'creates a comment on Jira'
+    context 'for issues' do
+      it_behaves_like 'handles cross-references' do
+        let(:resource) { build_stubbed(:issue, project: project) }
+        let(:comment_body) { /mentioned this issue in \[a issue\|/ }
       end
     end
 
-    context 'when resource is a merge request' do
-      let(:resource) { build_stubbed(:merge_request, source_project: project) }
-
-      context 'when disabled' do
-        before do
-          allow_next_instance_of(described_class) do |instance|
-            allow(instance).to receive(:merge_requests_events) { false }
-          end
-        end
+    context 'for merge requests' do
+      it_behaves_like 'handles cross-references' do
+        let(:resource) { build_stubbed(:merge_request, source_project: project) }
+        let(:comment_body) { /mentioned this issue in \[a merge request\|.* on branch \[master\|/ }
+      end
+    end
 
-        it { is_expected.to eq('Events for merge requests are disabled.') }
+    context 'for notes' do
+      it_behaves_like 'handles cross-references' do
+        let(:resource) { build_stubbed(:note, project: project) }
+        let(:comment_body) { /mentioned this issue in \[a note\|/ }
       end
+    end
 
-      context 'when enabled' do
-        it_behaves_like 'creates a comment on Jira'
+    context 'for snippets' do
+      it_behaves_like 'handles cross-references' do
+        let(:resource) { build_stubbed(:snippet, project: project) }
+        let(:comment_body) { /mentioned this issue in \[a snippet\|/ }
       end
     end
   end

spec/services/system_note_service_spec.rb

@@ -348,193 +348,6 @@ RSpec.describe SystemNoteService do
     end
   end
 
-  describe 'Jira integration' do
-    include JiraServiceHelper
-
-    let(:project)         { create(:jira_project, :repository) }
-    let(:author)          { create(:user) }
-    let(:issue)           { create(:issue, project: project) }
-    let(:merge_request)   { create(:merge_request, :simple, target_project: project, source_project: project) }
-    let(:jira_issue)      { ExternalIssue.new("JIRA-1", project)}
-    let(:jira_tracker)    { project.jira_integration }
-    let(:commit)          { project.commit }
-    let(:comment_url)     { jira_api_comment_url(jira_issue.id) }
-    let(:success_message) { "SUCCESS: Successfully posted to http://jira.example.net." }
-
-    before do
-      stub_jira_integration_test
-      stub_jira_urls(jira_issue.id)
-      jira_integration_settings
-    end
-
-    def cross_reference(type, link_exists = false)
-      noteable = type == 'commit' ? commit : merge_request
-
-      links = []
-      if link_exists
-        url = if type == 'commit'
-                "#{Settings.gitlab.base_url}/#{project.namespace.path}/#{project.path}/-/commit/#{commit.id}"
-              else
-                "#{Settings.gitlab.base_url}/#{project.namespace.path}/#{project.path}/-/merge_requests/#{merge_request.iid}"
-              end
-
-        link = double(object: { 'url' => url })
-        links << link
-        expect(link).to receive(:save!)
-      end
-
-      allow(JIRA::Resource::Remotelink).to receive(:all).and_return(links)
-
-      described_class.cross_reference(jira_issue, noteable, author)
-    end
-
-    noteable_types = %w(merge_requests commit)
-
-    noteable_types.each do |type|
-      context "when noteable is a #{type}" do
-        it "blocks cross reference when #{type.underscore}_events is false" do
-          jira_tracker.update!("#{type}_events" => false)
-
-          expect(cross_reference(type)).to eq(s_('JiraService|Events for %{noteable_model_name} are disabled.') % { noteable_model_name: type.pluralize.humanize.downcase })
-        end
-
-        it "creates cross reference when #{type.underscore}_events is true" do
-          jira_tracker.update!("#{type}_events" => true)
-
-          expect(cross_reference(type)).to eq(success_message)
-        end
-      end
-
-      context 'when a new cross reference is created' do
-        it 'creates a new comment and remote link' do
-          cross_reference(type)
-
-          expect(WebMock).to have_requested(:post, jira_api_comment_url(jira_issue))
-          expect(WebMock).to have_requested(:post, jira_api_remote_link_url(jira_issue))
-        end
-      end
-
-      context 'when a link exists' do
-        it 'updates a link but does not create a new comment' do
-          expect(WebMock).not_to have_requested(:post, jira_api_comment_url(jira_issue))
-
-          cross_reference(type, true)
-        end
-      end
-    end
-
-    describe "new reference" do
-      let(:favicon_path) { "http://localhost/assets/#{find_asset('favicon.png').digest_path}" }
-
-      before do
-        allow(JIRA::Resource::Remotelink).to receive(:all).and_return([])
-      end
-
-      context 'for commits' do
-        it "creates comment" do
-          result = described_class.cross_reference(jira_issue, commit, author)
-
-          expect(result).to eq(success_message)
-        end
-
-        it "creates remote link" do
-          described_class.cross_reference(jira_issue, commit, author)
-
-          expect(WebMock).to have_requested(:post, jira_api_remote_link_url(jira_issue)).with(
-            body: hash_including(
-              GlobalID: "GitLab",
-              relationship: 'mentioned on',
-              object: {
-                url: project_commit_url(project, commit),
-                title: "Commit - #{commit.title}",
-                icon: { title: "GitLab", url16x16: favicon_path },
-                status: { resolved: false }
-              }
-            )
-          ).once
-        end
-      end
-
-      context 'for issues' do
-        let(:issue) { create(:issue, project: project) }
-
-        it "creates comment" do
-          result = described_class.cross_reference(jira_issue, issue, author)
-
-          expect(result).to eq(success_message)
-        end
-
-        it "creates remote link" do
-          described_class.cross_reference(jira_issue, issue, author)
-
-          expect(WebMock).to have_requested(:post, jira_api_remote_link_url(jira_issue)).with(
-            body: hash_including(
-              GlobalID: "GitLab",
-              relationship: 'mentioned on',
-              object: {
-                url: project_issue_url(project, issue),
-                title: "Issue - #{issue.title}",
-                icon: { title: "GitLab", url16x16: favicon_path },
-                status: { resolved: false }
-              }
-            )
-          ).once
-        end
-      end
-
-      context 'for snippets' do
-        let(:snippet) { create(:snippet, project: project) }
-
-        it "creates comment" do
-          result = described_class.cross_reference(jira_issue, snippet, author)
-
-          expect(result).to eq(success_message)
-        end
-
-        it "creates remote link" do
-          described_class.cross_reference(jira_issue, snippet, author)
-
-          expect(WebMock).to have_requested(:post, jira_api_remote_link_url(jira_issue)).with(
-            body: hash_including(
-              GlobalID: "GitLab",
-              relationship: 'mentioned on',
-              object: {
-                url: project_snippet_url(project, snippet),
-                title: "Snippet - #{snippet.title}",
-                icon: { title: "GitLab", url16x16: favicon_path },
-                status: { resolved: false }
-              }
-            )
-          ).once
-        end
-      end
-    end
-
-    describe "existing reference" do
-      before do
-        allow(JIRA::Resource::Remotelink).to receive(:all).and_return([])
-        message = double('message')
-        allow(message).to receive(:include?) { true }
-        allow_next_instance_of(JIRA::Resource::Issue) do |instance|
-          allow(instance).to receive(:comments).and_return([OpenStruct.new(body: message)])
-        end
-      end
-
-      it "does not return success message" do
-        result = described_class.cross_reference(jira_issue, commit, author)
-
-        expect(result).not_to eq(success_message)
-      end
-
-      it 'does not try to create comment and remote link' do
-        subject
-
-        expect(WebMock).not_to have_requested(:post, jira_api_comment_url(jira_issue))
-        expect(WebMock).not_to have_requested(:post, jira_api_remote_link_url(jira_issue))
-      end
-    end
-  end
-
   describe '.change_time_estimate' do
     it 'calls TimeTrackingService' do
       expect_next_instance_of(::SystemNotes::TimeTrackingService) do |service|

spec/services/system_notes/issuables_service_spec.rb

@@ -347,6 +347,23 @@ RSpec.describe ::SystemNotes::IssuablesService do
           end
         end
       end
+
+      context 'with external issue' do
+        let(:noteable) { ExternalIssue.new('JIRA-123', project) }
+        let(:mentioner) { project.commit }
+
+        it 'queues a background worker' do
+          expect(Integrations::CreateExternalCrossReferenceWorker).to receive(:perform_async).with(
+            project.id,
+            'JIRA-123',
+            'Commit',
+            mentioner.id,
+            author.id
+          )
+
+          subject
+        end
+      end
     end
   end
 

spec/workers/integrations/create_external_cross_reference_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Integrations::CreateExternalCrossReferenceWorker do
+  include AfterNextHelpers
+  using RSpec::Parameterized::TableSyntax
+
+  let_it_be(:project) { create(:jira_project, :repository) }
+  let_it_be(:author) { create(:user) }
+  let_it_be(:commit) { project.commit }
+  let_it_be(:issue) { create(:issue, project: project) }
+  let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+  let_it_be(:note) { create(:note, project: project) }
+  let_it_be(:snippet) { create(:project_snippet, project: project) }
+
+  let(:project_id) { project.id }
+  let(:external_issue_id) { 'JIRA-123' }
+  let(:mentionable_type) { 'Issue' }
+  let(:mentionable_id) { issue.id }
+  let(:author_id) { author.id }
+  let(:job_args) { [project_id, external_issue_id, mentionable_type, mentionable_id, author_id] }
+
+  def perform
+    described_class.new.perform(*job_args)
+  end
+
+  before do
+    allow(Project).to receive(:find_by_id).and_return(project)
+  end
+
+  it_behaves_like 'an idempotent worker' do
+    before do
+      allow(project.external_issue_tracker).to receive(:create_cross_reference_note)
+    end
+
+    it 'can run multiple times with the same arguments' do
+      subject
+
+      expect(project.external_issue_tracker).to have_received(:create_cross_reference_note)
+        .exactly(worker_exec_times).times
+    end
+  end
+
+  it 'has the `until_executed` deduplicate strategy' do
+    expect(described_class.get_deduplicate_strategy).to eq(:until_executed)
+    expect(described_class.get_deduplication_options).to include({ including_scheduled: true })
+  end
+
+  # These are the only models where we currently support cross-references,
+  # although this should be expanded to all `Mentionable` models.
+  # See https://gitlab.com/gitlab-org/gitlab/-/issues/343975
+  where(:mentionable_type, :mentionable_id) do
+    'Commit'       | lazy { commit.id }
+    'Issue'        | lazy { issue.id }
+    'MergeRequest' | lazy { merge_request.id }
+    'Note'         | lazy { note.id }
+    'Snippet'      | lazy { snippet.id }
+  end
+
+  with_them do
+    it 'creates a cross reference' do
+      expect(project.external_issue_tracker).to receive(:create_cross_reference_note).with(
+        be_a(ExternalIssue).and(have_attributes(id: external_issue_id, project: project)),
+        be_a(mentionable_type.constantize).and(have_attributes(id: mentionable_id)),
+        be_a(User).and(have_attributes(id: author_id))
+      )
+
+      perform
+    end
+  end
+
+  describe 'error handling' do
+    shared_examples 'does not create a cross reference' do
+      it 'does not create a cross reference' do
+        expect(project).not_to receive(:external_issue_tracker) if project
+
+        perform
+      end
+    end
+
+    context 'project_id does not exist' do
+      let(:project_id) { non_existing_record_id }
+      let(:project) { nil }
+
+      it_behaves_like 'does not create a cross reference'
+    end
+
+    context 'author_id does not exist' do
+      let(:author_id) { non_existing_record_id }
+
+      it_behaves_like 'does not create a cross reference'
+    end
+
+    context 'mentionable_id does not exist' do
+      let(:mentionable_id) { non_existing_record_id }
+
+      it_behaves_like 'does not create a cross reference'
+    end
+
+    context 'mentionable_type is not a Mentionable' do
+      let(:mentionable_type) { 'User' }
+
+      before do
+        expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).with(kind_of(ArgumentError))
+      end
+
+      it_behaves_like 'does not create a cross reference'
+    end
+
+    context 'mentionable_type is not a defined constant' do
+      let(:mentionable_type) { 'FooBar' }
+
+      before do
+        expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).with(kind_of(ArgumentError))
+      end
+
+      it_behaves_like 'does not create a cross reference'
+    end
+
+    context 'mentionable is a Commit and mentionable_id does not exist' do
+      let(:mentionable_type) { 'Commit' }
+      let(:mentionable_id) { non_existing_record_id }
+
+      it_behaves_like 'does not create a cross reference'
+    end
+  end
+end