Merge branch '32844-issuables-performance' into 'master'

Issuables: Move some code from create services to Sidekiq workers

See merge request !13326

app/models/concerns/issuable.rb

@@ -16,6 +16,7 @@ module Issuable
   include TimeTrackable
   include Importable
   include Editable
+  include AfterCommitQueue
 
   # This object is used to gather issuable meta data for displaying
   # upvotes, downvotes, notes and closing merge requests count for issues and merge requests

app/services/issuable_base_service.rb

@@ -179,7 +179,6 @@ class IssuableBaseService < BaseService
 
     if params.present? && create_issuable(issuable, params, label_ids: label_ids)
       after_create(issuable)
-      issuable.create_cross_references!(current_user)
       execute_hooks(issuable)
       invalidate_cache_counts(issuable, users: issuable.assignees)
     end

app/services/issues/create_service.rb

@@ -15,11 +15,14 @@ module Issues
     def before_create(issue)
       spam_check(issue, current_user)
       issue.move_to_end
+
+      user = current_user
+      issue.run_after_commit do
+        NewIssueWorker.perform_async(issue.id, user.id)
+      end
     end
 
     def after_create(issuable)
-      event_service.open_issue(issuable, current_user)
-      notification_service.new_issue(issuable, current_user)
       todo_service.new_issue(issuable, current_user)
       user_agent_detail_service.create
       resolve_discussions_with_issue(issuable)

app/services/merge_requests/create_service.rb

@@ -16,9 +16,15 @@ module MergeRequests
       create(merge_request)
     end
 
+    def before_create(merge_request)
+      user = current_user
+      merge_request.run_after_commit do
+        NewMergeRequestWorker.perform_async(merge_request.id, user.id)
+      end
+    end
+
     def after_create(issuable)
       event_service.open_mr(issuable, current_user)
-      notification_service.new_merge_request(issuable, current_user)
       todo_service.new_merge_request(issuable, current_user)
       issuable.cache_merge_request_closes_issues!(current_user)
       update_merge_requests_head_pipeline(issuable)

app/workers/concerns/new_issuable.rb

+module NewIssuable
+  attr_reader :issuable, :user
+
+  def ensure_objects_found(issuable_id, user_id)
+    @issuable = issuable_class.find_by(id: issuable_id)
+    unless @issuable
+      log_error(issuable_class, issuable_id)
+      return false
+    end
+
+    @user = User.find_by(id: user_id)
+    unless @user
+      log_error(User, user_id)
+      return false
+    end
+
+    true
+  end
+
+  def log_error(record_class, record_id)
+    Rails.logger.error("#{self.class}: couldn't find #{record_class} with ID=#{record_id}, skipping job")
+  end
+end

app/workers/new_issue_worker.rb

+class NewIssueWorker
+  include Sidekiq::Worker
+  include DedicatedSidekiqQueue
+  include NewIssuable
+
+  def perform(issue_id, user_id)
+    return unless ensure_objects_found(issue_id, user_id)
+
+    EventCreateService.new.open_issue(issuable, user)
+    NotificationService.new.new_issue(issuable, user)
+    issuable.create_cross_references!(user)
+  end
+
+  def issuable_class
+    Issue
+  end
+end

app/workers/new_merge_request_worker.rb

+class NewMergeRequestWorker
+  include Sidekiq::Worker
+  include DedicatedSidekiqQueue
+  include NewIssuable
+
+  def perform(merge_request_id, user_id)
+    return unless ensure_objects_found(merge_request_id, user_id)
+
+    EventCreateService.new.open_mr(issuable, user)
+    NotificationService.new.new_merge_request(issuable, user)
+    issuable.create_cross_references!(user)
+  end
+
+  def issuable_class
+    MergeRequest
+  end
+end

changelogs/unreleased/32844-issuables-performance.yml

+---
+title: Move some code from services to workers in order to improve performance
+merge_request: 13326
+author:

config/sidekiq_queues.yml

@@ -23,6 +23,8 @@
   - [update_merge_requests, 3]
   - [process_commit, 3]
   - [new_note, 2]
+  - [new_issue, 2]
+  - [new_merge_request, 2]
   - [build, 2]
   - [pipeline, 2]
   - [gitlab_shell, 2]

spec/features/issues/create_branch_merge_request_spec.rb

 require 'rails_helper'
 
-feature 'Create Branch/Merge Request Dropdown on issue page', js: true do
+feature 'Create Branch/Merge Request Dropdown on issue page', :feature, :js do
   let(:user) { create(:user) }
   let!(:project) { create(:project, :repository) }
   let(:issue) { create(:issue, project: project, title: 'Cherry-Coloured Funk') }
@@ -14,10 +14,14 @@ feature 'Create Branch/Merge Request Dropdown on issue page', js: true do
     it 'allows creating a merge request from the issue page' do
       visit project_issue_path(project, issue)
 
-      select_dropdown_option('create-mr')
-      
-      expect(page).to have_content('WIP: Resolve "Cherry-Coloured Funk"')
-      expect(current_path).to eq(project_merge_request_path(project, MergeRequest.first))
+      perform_enqueued_jobs do
+        select_dropdown_option('create-mr')
+
+        expect(page).to have_content('WIP: Resolve "Cherry-Coloured Funk"')
+        expect(current_path).to eq(project_merge_request_path(project, MergeRequest.first))
+
+        wait_for_requests
+      end
 
       visit project_issue_path(project, issue)
 

spec/models/issue_spec.rb

@@ -191,14 +191,10 @@ describe Issue do
     end
 
     it 'returns the merge request to close this issue' do
-      mr
-
       expect(issue.closed_by_merge_requests(mr.author)).to eq([mr])
     end
 
     it "returns an empty array when the merge request is closed already" do
-      closed_mr
-
       expect(issue.closed_by_merge_requests(closed_mr.author)).to eq([])
     end
 

spec/support/cycle_analytics_helpers.rb

@@ -41,7 +41,9 @@ module CycleAnalyticsHelpers
       target_branch: 'master'
     }
 
-    MergeRequests::CreateService.new(project, user, opts).execute
+    mr = MergeRequests::CreateService.new(project, user, opts).execute
+    NewMergeRequestWorker.new.perform(mr, user)
+    mr
   end
 
   def merge_merge_requests_closing_issue(issue)

spec/workers/new_issue_worker_spec.rb

+require 'spec_helper'
+
+describe NewIssueWorker do
+  describe '#perform' do
+    let(:worker) { described_class.new }
+
+    context 'when an issue not found' do
+      it 'does not call Services' do
+        expect(EventCreateService).not_to receive(:new)
+        expect(NotificationService).not_to receive(:new)
+
+        worker.perform(99, create(:user).id)
+      end
+
+      it 'logs an error' do
+        expect(Rails.logger).to receive(:error).with('NewIssueWorker: couldn\'t find Issue with ID=99, skipping job')
+
+        worker.perform(99, create(:user).id)
+      end
+    end
+
+    context 'when a user not found' do
+      it 'does not call Services' do
+        expect(EventCreateService).not_to receive(:new)
+        expect(NotificationService).not_to receive(:new)
+
+        worker.perform(create(:issue).id, 99)
+      end
+
+      it 'logs an error' do
+        expect(Rails.logger).to receive(:error).with('NewIssueWorker: couldn\'t find User with ID=99, skipping job')
+
+        worker.perform(create(:issue).id, 99)
+      end
+    end
+
+    context 'when everything is ok' do
+      let(:project) { create(:project, :public) }
+      let(:mentioned) { create(:user) }
+      let(:user) { create(:user) }
+      let(:issue) { create(:issue, project: project, description: "issue for #{mentioned.to_reference}") }
+
+      it 'creates a new event record' do
+        expect{ worker.perform(issue.id, user.id) }.to change { Event.count }.from(0).to(1)
+      end
+
+      it 'creates a notification for the assignee' do
+        expect(Notify).to receive(:new_issue_email).with(mentioned.id, issue.id).and_return(double(deliver_later: true))
+
+        worker.perform(issue.id, user.id)
+      end
+    end
+  end
+end

spec/workers/new_merge_request_worker_spec.rb

+require 'spec_helper'
+
+describe NewMergeRequestWorker do
+  describe '#perform' do
+    let(:worker) { described_class.new }
+
+    context 'when a merge request not found' do
+      it 'does not call Services' do
+        expect(EventCreateService).not_to receive(:new)
+        expect(NotificationService).not_to receive(:new)
+
+        worker.perform(99, create(:user).id)
+      end
+
+      it 'logs an error' do
+        expect(Rails.logger).to receive(:error).with('NewMergeRequestWorker: couldn\'t find MergeRequest with ID=99, skipping job')
+
+        worker.perform(99, create(:user).id)
+      end
+    end
+
+    context 'when a user not found' do
+      it 'does not call Services' do
+        expect(EventCreateService).not_to receive(:new)
+        expect(NotificationService).not_to receive(:new)
+
+        worker.perform(create(:merge_request).id, 99)
+      end
+
+      it 'logs an error' do
+        expect(Rails.logger).to receive(:error).with('NewMergeRequestWorker: couldn\'t find User with ID=99, skipping job')
+
+        worker.perform(create(:merge_request).id, 99)
+      end
+    end
+
+    context 'when everything is ok' do
+      let(:project) { create(:project, :public) }
+      let(:mentioned) { create(:user) }
+      let(:user) { create(:user) }
+      let(:merge_request) do
+        create(:merge_request, source_project: project, description: "mr for #{mentioned.to_reference}")
+      end
+
+      it 'creates a new event record' do
+        expect{ worker.perform(merge_request.id, user.id) }.to change { Event.count }.from(0).to(1)
+      end
+
+      it 'creates a notification for the assignee' do
+        expect(Notify).to receive(:new_merge_request_email).with(mentioned.id, merge_request.id).and_return(double(deliver_later: true))
+
+        worker.perform(merge_request.id, user.id)
+      end
+    end
+  end
+end