Merge branch 'refresh-service-optimisations' into 'master'

Move approval reset to new service and worker

See merge request gitlab-org/gitlab!42001

app/workers/update_merge_requests_worker.rb

@@ -9,8 +9,6 @@ class UpdateMergeRequestsWorker # rubocop:disable Scalability/IdempotentWorker
   weight 3
   loggable_arguments 2, 3, 4
 
-  LOG_TIME_THRESHOLD = 90 # seconds
-
   # rubocop: disable CodeReuse/ActiveRecord
   def perform(project_id, user_id, oldrev, newrev, ref)
     project = Project.find_by(id: project_id)

config/sidekiq_queues.yml

@@ -160,6 +160,8 @@
   - 1
 - - merge_request_mergeability_check
   - 1
+- - merge_request_reset_approvals
+  - 1
 - - metrics_dashboard_prune_old_annotations
   - 1
 - - migrate_external_diffs

ee/app/services/ee/merge_requests/refresh_service.rb

@@ -18,14 +18,8 @@ module EE
         reset_approvals_for_merge_requests(push.ref, push.newrev)
       end
 
-      # Note: Closed merge requests also need approvals reset.
       def reset_approvals_for_merge_requests(ref, newrev)
-        branch_name = ::Gitlab::Git.ref_name(ref)
-        merge_requests = merge_requests_for(branch_name, mr_states: [:opened, :closed])
-
-        merge_requests.each do |merge_request|
-          reset_approvals(merge_request, newrev)
-        end
+        MergeRequestResetApprovalsWorker.perform_async(project.id, current_user.id, ref, newrev)
       end
 
       # @return [Hash<Integer, MergeRequestDiff>] Diffs prior to code push, mapped from merge request id
@@ -78,10 +72,6 @@ module EE
           .including_merge_train
       end
       # rubocop:enable Gitlab/ModuleWithInstanceVariables
-
-      def reset_approvals?(merge_request, newrev)
-        super && merge_request.rebase_commit_sha != newrev
-      end
     end
   end
 end

ee/app/services/ee/merge_requests/reset_approvals_service.rb

+# frozen_string_literal: true
+
+module EE
+  module MergeRequests
+    class ResetApprovalsService < ::MergeRequests::BaseService
+      def execute(ref, newrev)
+        reset_approvals_for_merge_requests(ref, newrev)
+      end
+
+      private
+
+      # Note: Closed merge requests also need approvals reset.
+      def reset_approvals_for_merge_requests(ref, newrev)
+        branch_name = ::Gitlab::Git.ref_name(ref)
+        merge_requests = merge_requests_for(branch_name, mr_states: [:opened, :closed])
+
+        merge_requests.each do |merge_request|
+          reset_approvals(merge_request, newrev)
+        end
+      end
+
+      def reset_approvals?(merge_request, newrev)
+        super && merge_request.rebase_commit_sha != newrev
+      end
+    end
+  end
+end

ee/app/workers/all_queues.yml

@@ -635,6 +635,14 @@
   :weight: 2
   :idempotent:
   :tags: []
+- :name: merge_request_reset_approvals
+  :feature_category: :source_code_management
+  :has_external_dependencies:
+  :urgency: :high
+  :resource_boundary: :cpu
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: new_epic
   :feature_category: :epics
   :has_external_dependencies:

ee/app/workers/merge_request_reset_approvals_worker.rb

+# frozen_string_literal: true
+
+class MergeRequestResetApprovalsWorker # rubocop:disable Scalability/IdempotentWorker
+  include ApplicationWorker
+
+  feature_category :source_code_management
+  urgency :high
+  worker_resource_boundary :cpu
+  loggable_arguments 2, 3
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def perform(project_id, user_id, ref, newrev)
+    project = Project.find_by(id: project_id)
+    return unless project
+
+    user = User.find_by(id: user_id)
+    return unless user
+
+    EE::MergeRequests::ResetApprovalsService.new(project, user).execute(ref, newrev)
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+end

ee/changelogs/unreleased/refresh-service-optimisations.yml

+---
+title: Move approval reset to new service and worker
+merge_request: 42001
+author:
+type: performance

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -13,6 +13,7 @@ RSpec.describe MergeRequests::RefreshService do
   let(:fork_user) { create(:user) }
 
   let(:source_branch) { 'between-create-delete-modify-move' }
+
   let(:merge_request) do
     create(:merge_request,
       source_project: project,
@@ -342,7 +343,7 @@ RSpec.describe MergeRequests::RefreshService do
         Todo.where(action: Todo::APPROVAL_REQUIRED, target: merge_request)
       end
 
-      context 'push to origin repo source branch' do
+      context 'push to origin repo source branch', :sidekiq_inline do
         let(:notification_service) { spy('notification_service') }
 
         before do
@@ -444,7 +445,7 @@ RSpec.describe MergeRequests::RefreshService do
         end
       end
 
-      context 'resetting approvals if they are enabled' do
+      context 'resetting approvals if they are enabled', :sidekiq_inline do
         context 'when approvals_before_merge is disabled' do
           before do
             project.update(approvals_before_merge: 0)
@@ -487,7 +488,7 @@ RSpec.describe MergeRequests::RefreshService do
           end
         end
 
-        context 'when there are approvals' do
+        context 'when there are approvals', :sidekiq_inline do
           context 'closed merge request' do
             before do
               merge_request.close!

ee/spec/services/ee/merge_requests/reset_approvals_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe EE::MergeRequests::ResetApprovalsService do
+  let(:service) { described_class.new(project, current_user) }
+  let(:current_user) { merge_request.author }
+  let(:group) { create(:group) }
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :repository, namespace: group, approvals_before_merge: 1, reset_approvals_on_push: true) }
+
+  let(:merge_request) do
+    create(:merge_request,
+      source_project: project,
+      source_branch: 'master',
+      target_branch: 'feature',
+      target_project: project,
+      merge_when_pipeline_succeeds: true,
+      merge_user: user)
+  end
+
+  let(:commits) { merge_request.commits }
+  let(:oldrev) { commits.last.id }
+  let(:newrev) { commits.first.id }
+  let(:approver) { create(:user) }
+  let(:notification_service) { spy('notification_service') }
+
+  def approval_todos(merge_request)
+    Todo.where(action: Todo::APPROVAL_REQUIRED, target: merge_request)
+  end
+
+  describe "#execute" do
+    before do
+      allow(service).to receive(:execute_hooks)
+      allow(NotificationService).to receive(:new) { notification_service }
+      project.add_developer(approver)
+
+      perform_enqueued_jobs do
+        merge_request.update!(approver_ids: [approver].map(&:id).join(','))
+      end
+
+      merge_request.approvals.create!(user_id: approver.id)
+    end
+
+    it 'resets approvals' do
+      service.execute("refs/heads/master", newrev)
+      merge_request.reload
+
+      expect(merge_request.approvals).to be_empty
+      expect(approval_todos(merge_request).map(&:user)).to contain_exactly(approver)
+    end
+  end
+end

ee/spec/workers/merge_request_reset_approvals_worker_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe MergeRequestResetApprovalsWorker do
+  include RepoHelpers
+
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+
+  subject { described_class.new }
+
+  describe "#perform" do
+    let(:newrev) { "789012" }
+    let(:ref)    { "refs/heads/test" }
+
+    def perform
+      subject.perform(project.id, user.id, ref, newrev)
+    end
+
+    it "executes MergeRequests::RefreshService with expected values" do
+      expect_next_instance_of(EE::MergeRequests::ResetApprovalsService, project, user) do |refresh_service|
+        expect(refresh_service).to receive(:execute).with(ref, newrev)
+      end
+
+      perform
+    end
+
+    context "project is missing" do
+      let(:project) { double("project", id: "foo") }
+
+      it "doesn't execute the service" do
+        expect(EE::MergeRequests::ResetApprovalsService).not_to receive(:new)
+
+        perform
+      end
+    end
+
+    context "user is missing" do
+      let(:user) { double("user", id: "foo") }
+
+      it "doesn't execute the service" do
+        expect(EE::MergeRequests::ResetApprovalsService).not_to receive(:new)
+
+        perform
+      end
+    end
+  end
+end