Merge branch 'mk/speed-up-merge-request-creation-action' into 'master'

Improve merge request creation response time by performing more tasks asynchronously [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57453

app/controllers/projects/merge_requests/creations_controller.rb

@@ -6,7 +6,6 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
   include RendersCommits
 
   skip_before_action :merge_request
-  before_action :disable_query_limiting, only: [:create]
   before_action :authorize_create_merge_request_from!
   before_action :apply_diff_view_cookie!, only: [:diffs, :diff_for_path]
   before_action :build_merge_request, except: [:create]
@@ -133,13 +132,11 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
-  def disable_query_limiting
-    Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/20801')
-  end
-
   def incr_count_webide_merge_request
     return if params[:nav_source] != 'webide'
 
     Gitlab::UsageDataCounters::WebIdeCounter.increment_merge_requests_count
   end
 end
+
+Projects::MergeRequests::CreationsController.prepend_ee_mod

app/services/merge_requests/after_create_service.rb

@@ -24,6 +24,16 @@ module MergeRequests
       merge_request.create_cross_references!(current_user)
 
       OnboardingProgressService.new(merge_request.target_project.namespace).execute(action: :merge_request_created)
+
+      todo_service.new_merge_request(merge_request, current_user)
+      merge_request.cache_merge_request_closes_issues!(current_user)
+
+      Gitlab::UsageDataCounters::MergeRequestCounter.count(:create)
+      link_lfs_objects(merge_request)
+    end
+
+    def link_lfs_objects(merge_request)
+      LinkLfsObjectsService.new(merge_request.target_project).execute(merge_request)
     end
   end
 end

app/services/merge_requests/create_service.rb

@@ -18,12 +18,6 @@ module MergeRequests
       # be performed in Sidekiq
       NewMergeRequestWorker.perform_async(issuable.id, current_user.id)
 
-      todo_service.new_merge_request(issuable, current_user)
-      issuable.cache_merge_request_closes_issues!(current_user)
-
-      Gitlab::UsageDataCounters::MergeRequestCounter.count(:create)
-      link_lfs_objects(issuable)
-
       super
     end
 
@@ -54,10 +48,6 @@ module MergeRequests
         raise Gitlab::Access::AccessDeniedError
       end
     end
-
-    def link_lfs_objects(issuable)
-      LinkLfsObjectsService.new(issuable.target_project).execute(issuable)
-    end
   end
 end
 

changelogs/unreleased/mk-speed-up-merge-request-creation-action.yml

+---
+title: Perform more merge request creation tasks asynchronously to improve response
+  times
+merge_request: 57453
+author:
+type: performance

ee/app/controllers/ee/projects/merge_requests/creations_controller.rb

+# frozen_string_literal: true
+
+module EE
+  module Projects
+    module MergeRequests
+      module CreationsController
+        extend ActiveSupport::Concern
+
+        prepended do
+          before_action :disable_query_limiting, only: [:create]
+        end
+
+        private
+
+        def disable_query_limiting
+          ::Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/20801')
+        end
+      end
+    end
+  end
+end

ee/spec/controllers/projects/merge_requests/creations_controller_spec.rb

@@ -139,5 +139,11 @@ RSpec.describe Projects::MergeRequests::CreationsController do
         end
       end
     end
+
+    it 'disables query limiting' do
+      expect(Gitlab::QueryLimiting).to receive(:disable!)
+
+      create_merge_request
+    end
   end
 end

spec/services/merge_requests/after_create_service_spec.rb

@@ -93,5 +93,93 @@ RSpec.describe MergeRequests::AfterCreateService do
         expect(merge_request.reload).to be_unchecked
       end
     end
+
+    it 'increments the usage data counter of create event' do
+      counter = Gitlab::UsageDataCounters::MergeRequestCounter
+
+      expect { execute_service }.to change { counter.read(:create) }.by(1)
+    end
+
+    context 'todos' do
+      it 'does not creates todos' do
+        attributes = {
+          project: merge_request.target_project,
+          target_id: merge_request.id,
+          target_type: merge_request.class.name
+        }
+
+        expect { execute_service }.not_to change { Todo.where(attributes).count }
+      end
+
+      context 'when merge request is assigned to someone' do
+        let_it_be(:assignee) { create(:user) }
+        let_it_be(:merge_request) { create(:merge_request, assignees: [assignee]) }
+
+        it 'creates a todo for new assignee' do
+          attributes = {
+            project: merge_request.target_project,
+            author: merge_request.author,
+            user: assignee,
+            target_id: merge_request.id,
+            target_type: merge_request.class.name,
+            action: Todo::ASSIGNED,
+            state: :pending
+          }
+
+          expect { execute_service }.to change { Todo.where(attributes).count }.by(1)
+        end
+      end
+
+      context 'when reviewer is assigned' do
+        let_it_be(:reviewer) { create(:user) }
+        let_it_be(:merge_request) { create(:merge_request, reviewers: [reviewer]) }
+
+        it 'creates a todo for new reviewer' do
+          attributes = {
+            project: merge_request.target_project,
+            author: merge_request.author,
+            user: reviewer,
+            target_id: merge_request.id,
+            target_type: merge_request.class.name,
+            action: Todo::REVIEW_REQUESTED,
+            state: :pending
+          }
+
+          expect { execute_service }.to change { Todo.where(attributes).count }.by(1)
+        end
+      end
+    end
+
+    context 'when saving references to issues that the created merge request closes' do
+      let_it_be(:first_issue) { create(:issue, project: merge_request.target_project) }
+      let_it_be(:second_issue) { create(:issue, project: merge_request.target_project) }
+
+      it 'creates a `MergeRequestsClosingIssues` record for each issue' do
+        merge_request.description = "Closes #{first_issue.to_reference} and #{second_issue.to_reference}"
+        merge_request.source_branch = "feature"
+        merge_request.target_branch = merge_request.target_project.default_branch
+        merge_request.save!
+
+        execute_service
+
+        issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id)
+        expect(issue_ids).to match_array([first_issue.id, second_issue.id])
+      end
+    end
+
+    it 'tracks merge request creation in usage data' do
+      expect(Gitlab::UsageDataCounters::MergeRequestCounter).to receive(:count).with(:create)
+
+      execute_service
+    end
+
+    it 'calls MergeRequests::LinkLfsObjectsService#execute' do
+      service = instance_spy(MergeRequests::LinkLfsObjectsService)
+      allow(MergeRequests::LinkLfsObjectsService).to receive(:new).with(merge_request.target_project).and_return(service)
+
+      execute_service
+
+      expect(service).to have_received(:execute).with(merge_request)
+    end
   end
 end

spec/services/merge_requests/create_service_spec.rb

@@ -47,16 +47,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
           .to change { project.open_merge_requests_count }.from(0).to(1)
       end
 
-      it 'does not creates todos' do
-        attributes = {
-          project: project,
-          target_id: merge_request.id,
-          target_type: merge_request.class.name
-        }
-
-        expect(Todo.where(attributes).count).to be_zero
-      end
-
       it 'creates exactly 1 create MR event', :sidekiq_might_not_need_inline do
         attributes = {
           action: :created,
@@ -113,20 +103,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
         end
 
         it { expect(merge_request.assignees).to eq([user2]) }
-
-        it 'creates a todo for new assignee' do
-          attributes = {
-            project: project,
-            author: user,
-            user: user2,
-            target_id: merge_request.id,
-            target_type: merge_request.class.name,
-            action: Todo::ASSIGNED,
-            state: :pending
-          }
-
-          expect(Todo.where(attributes).count).to eq 1
-        end
       end
 
       context 'when reviewer is assigned' do
@@ -142,20 +118,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
 
         it { expect(merge_request.reviewers).to eq([user2]) }
 
-        it 'creates a todo for new reviewer' do
-          attributes = {
-            project: project,
-            author: user,
-            user: user2,
-            target_id: merge_request.id,
-            target_type: merge_request.class.name,
-            action: Todo::REVIEW_REQUESTED,
-            state: :pending
-          }
-
-          expect(Todo.where(attributes).count).to eq 1
-        end
-
         it 'invalidates counter cache for reviewers', :use_clean_rails_memory_store_caching do
           expect { merge_request }
             .to change { user2.review_requested_open_merge_requests_count }
@@ -328,12 +290,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
         end
       end
 
-      it 'increments the usage data counter of create event' do
-        counter = Gitlab::UsageDataCounters::MergeRequestCounter
-
-        expect { service.execute }.to change { counter.read(:create) }.by(1)
-      end
-
       context 'after_save callback to store_mentions' do
         let(:labels) { create_pair(:label, project: project) }
         let(:milestone) { create(:milestone, project: project) }
@@ -494,35 +450,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
       end
     end
 
-    context 'while saving references to issues that the created merge request closes' do
-      let(:first_issue) { create(:issue, project: project) }
-      let(:second_issue) { create(:issue, project: project) }
-
-      let(:opts) do
-        {
-          title: 'Awesome merge_request',
-          source_branch: 'feature',
-          target_branch: 'master',
-          force_remove_source_branch: '1'
-        }
-      end
-
-      before do
-        project.add_maintainer(user)
-        project.add_developer(user2)
-      end
-
-      it 'creates a `MergeRequestsClosingIssues` record for each issue' do
-        issue_closing_opts = opts.merge(description: "Closes #{first_issue.to_reference} and #{second_issue.to_reference}")
-        service = described_class.new(project, user, issue_closing_opts)
-        allow(service).to receive(:execute_hooks)
-        merge_request = service.execute
-
-        issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id)
-        expect(issue_ids).to match_array([first_issue.id, second_issue.id])
-      end
-    end
-
     context 'when source and target projects are different' do
       let(:target_project) { fork_project(project, nil, repository: true) }
 
@@ -571,14 +498,6 @@ RSpec.describe MergeRequests::CreateService, :clean_gitlab_redis_shared_state do
           expect(merge_request).to be_persisted
         end
 
-        it 'calls MergeRequests::LinkLfsObjectsService#execute', :sidekiq_might_not_need_inline do
-          expect_next_instance_of(MergeRequests::LinkLfsObjectsService) do |service|
-            expect(service).to receive(:execute).with(instance_of(MergeRequest))
-          end
-
-          described_class.new(project, user, opts).execute
-        end
-
         it 'does not create the merge request when the target project is archived' do
           target_project.update!(archived: true)
 

spec/services/merge_requests/update_service_spec.rb

@@ -948,18 +948,8 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
       end
 
       it 'removes `MergeRequestsClosingIssues` records when issues are not closed anymore' do
-        opts = {
-          title: 'Awesome merge_request',
-          description: "Closes #{first_issue.to_reference} and #{second_issue.to_reference}",
-          source_branch: 'feature',
-          target_branch: 'master',
-          force_remove_source_branch: '1'
-        }
-
-        merge_request = MergeRequests::CreateService.new(project, user, opts).execute
-
-        issue_ids = MergeRequestsClosingIssues.where(merge_request: merge_request).pluck(:issue_id)
-        expect(issue_ids).to match_array([first_issue.id, second_issue.id])
+        create(:merge_requests_closing_issues, issue: first_issue, merge_request: merge_request)
+        create(:merge_requests_closing_issues, issue: second_issue, merge_request: merge_request)
 
         service = described_class.new(project, user, description: "not closing any issues")
         allow(service).to receive(:execute_hooks)