Merge branch 'kerrizor/use-specialized-service-for-assignee-updates' into 'master'

Use specialized services for faster updates

See merge request gitlab-org/gitlab!58836

app/services/merge_requests/update_service.rb

@@ -11,18 +11,7 @@ module MergeRequests
     end
 
     def execute(merge_request)
-      # We don't allow change of source/target projects and source branch
-      # after merge request was created
-      params.delete(:source_project_id)
-      params.delete(:target_project_id)
-      params.delete(:source_branch)
-
-      if merge_request.closed_or_merged_without_fork?
-        params.delete(:target_branch)
-        params.delete(:force_remove_source_branch)
-      end
-
-      update_task_event(merge_request) || update(merge_request)
+      update_merge_request_with_specialized_service(merge_request) || general_fallback(merge_request)
     end
 
     def handle_changes(merge_request, options)
@@ -86,6 +75,21 @@ module MergeRequests
 
     attr_reader :target_branch_was_deleted
 
+    def general_fallback(merge_request)
+      # We don't allow change of source/target projects and source branch
+      # after merge request was created
+      params.delete(:source_project_id)
+      params.delete(:target_project_id)
+      params.delete(:source_branch)
+
+      if merge_request.closed_or_merged_without_fork?
+        params.delete(:target_branch)
+        params.delete(:force_remove_source_branch)
+      end
+
+      update_task_event(merge_request) || update(merge_request)
+    end
+
     def track_title_and_desc_edits(changed_fields)
       tracked_fields = %w(title description)
 
@@ -272,6 +276,34 @@ module MergeRequests
     def quick_action_options
       { merge_request_diff_head_sha: params.delete(:merge_request_diff_head_sha) }
     end
+
+    def update_merge_request_with_specialized_service(merge_request)
+      return unless params.delete(:use_specialized_service)
+
+      # If we're attempting to modify only a single attribute, look up whether
+      #   we have a specialized, targeted service we should use instead. We may
+      #   in the future extend this to include specialized services that operate
+      #   on multiple attributes, but for now limit to only single attribute
+      #   updates.
+      #
+      return unless params.one?
+
+      attempt_specialized_update_services(merge_request, params.each_key.first.to_sym)
+    end
+
+    def attempt_specialized_update_services(merge_request, attribute)
+      case attribute
+      when :assignee_ids
+        assignees_service.execute(merge_request)
+      else
+        nil
+      end
+    end
+
+    def assignees_service
+      @assignees_service ||= ::MergeRequests::UpdateAssigneesService
+        .new(project, current_user, params)
+    end
   end
 end
 

changelogs/unreleased/kerrizor-use-specialized-service-for-assignee-updates.yml

+---
+title: Add framework for using specialized services to improve performance of MergeRequests::UpdateService
+merge_request: 58836
+author:
+type: performance

ee/app/services/ee/merge_requests/update_service.rb

@@ -5,8 +5,10 @@ module EE
     module UpdateService
       extend ::Gitlab::Utils::Override
 
-      override :execute
-      def execute(merge_request)
+      private
+
+      override :general_fallback
+      def general_fallback(merge_request)
         unless update_task_event?
           old_approvers = merge_request.overall_approvers(exclude_code_owners: true)
         end
@@ -33,8 +35,6 @@ module EE
         merge_request
       end
 
-      private
-
       override :after_update
       def after_update(merge_request)
         super

spec/services/merge_requests/update_service_spec.rb

@@ -955,6 +955,40 @@ RSpec.describe MergeRequests::UpdateService, :mailer do
     end
 
     context 'updating asssignee_ids' do
+      context ':use_specialized_service' do
+        context 'when true' do
+          it 'passes the update action to ::MergeRequests::UpdateAssigneesService' do
+            expect(::MergeRequests::UpdateAssigneesService)
+              .to receive(:new).and_call_original
+
+            update_merge_request({
+              assignee_ids: [user2.id],
+              use_specialized_service: true
+            })
+          end
+        end
+
+        context 'when false or nil' do
+          before do
+            expect(::MergeRequests::UpdateAssigneesService).not_to receive(:new)
+          end
+
+          it 'does not pass the update action to ::MergeRequests::UpdateAssigneesService when false' do
+            update_merge_request({
+              assignee_ids: [user2.id],
+              use_specialized_service: false
+            })
+          end
+
+          it 'does not pass the update action to ::MergeRequests::UpdateAssigneesService when nil' do
+            update_merge_request({
+              assignee_ids: [user2.id],
+              use_specialized_service: nil
+            })
+          end
+        end
+      end
+
       it 'does not update assignee when assignee_id is invalid' do
         merge_request.update!(assignee_ids: [user.id])