Merge branch 'infra_6609_move_statistics_update_to_prevent_long_running_transactions' into 'master'

Move updating statistics logic outside of the transaction

See merge request gitlab-org/gitlab!83018

ee/app/services/vulnerabilities/base_service.rb

@@ -12,14 +12,29 @@ module Vulnerabilities
 
     private
 
-    def update_with_note(vulnerability, params)
-      return false unless vulnerability.update(params)
+    def update_vulnerability_with(params)
+      @vulnerability.transaction do
+        yield if block_given?
 
-      Vulnerabilities::Statistics::UpdateService.update_for(vulnerability)
-      SystemNoteService.change_vulnerability_state(vulnerability, @user) if vulnerability.state_previously_changed?
+        update_with_note(params)
+      end
+
+      update_statistics
+    end
+
+    def update_with_note(params)
+      return false unless @vulnerability.update(params)
+
+      # The following service call alters the `previous_changes` of the vulnerability object
+      # therefore, we are sending the cloned object as that information is important for the rest of the logic.
+      SystemNoteService.change_vulnerability_state(@vulnerability.clone, @user) if @vulnerability.state_previously_changed?
       true
     end
 
+    def update_statistics
+      Vulnerabilities::Statistics::UpdateService.update_for(@vulnerability) if @vulnerability.previous_changes.present?
+    end
+
     def authorized?
       can?(@user, :admin_vulnerability, @project)
     end

ee/app/services/vulnerabilities/confirm_service.rb

@@ -7,10 +7,8 @@ module Vulnerabilities
     def execute
       raise Gitlab::Access::AccessDeniedError unless authorized?
 
-      @vulnerability.transaction do
+      update_vulnerability_with(state: Vulnerability.states[:confirmed], confirmed_by: @user, confirmed_at: Time.current) do
         DestroyDismissalFeedbackService.new(@user, @vulnerability).execute
-
-        update_with_note(@vulnerability, state: Vulnerability.states[:confirmed], confirmed_by: @user, confirmed_at: Time.current)
       end
 
       @vulnerability

ee/app/services/vulnerabilities/dismiss_service.rb

@@ -16,7 +16,7 @@ module Vulnerabilities
     def execute
       raise Gitlab::Access::AccessDeniedError unless authorized?
 
-      @vulnerability.transaction do
+      update_vulnerability_with(state: Vulnerability.states[:dismissed], dismissed_by: @user, dismissed_at: Time.current) do
         if dismiss_findings
           result = dismiss_vulnerability_findings
 
@@ -25,8 +25,6 @@ module Vulnerabilities
             raise ActiveRecord::Rollback
           end
         end
-
-        update_with_note(@vulnerability, state: Vulnerability.states[:dismissed], dismissed_by: @user, dismissed_at: Time.current)
       end
 
       @vulnerability

ee/app/services/vulnerabilities/resolve_service.rb

@@ -7,10 +7,8 @@ module Vulnerabilities
     def execute
       raise Gitlab::Access::AccessDeniedError unless authorized?
 
-      @vulnerability.transaction do
+      update_vulnerability_with(state: Vulnerability.states[:resolved], resolved_by: @user, resolved_at: Time.current) do
         DestroyDismissalFeedbackService.new(@user, @vulnerability).execute
-
-        update_with_note(@vulnerability, state: Vulnerability.states[:resolved], resolved_by: @user, resolved_at: Time.current)
       end
 
       @vulnerability

ee/app/services/vulnerabilities/revert_to_detected_service.rb

@@ -9,10 +9,8 @@ module Vulnerabilities
     def execute
       raise Gitlab::Access::AccessDeniedError unless authorized?
 
-      @vulnerability.transaction do
+      update_vulnerability_with(state: Vulnerability.states[:detected], **REVERT_PARAMS) do
         DestroyDismissalFeedbackService.new(@user, @vulnerability).execute
-
-        update_with_note(@vulnerability, state: Vulnerability.states[:detected], **REVERT_PARAMS)
       end
 
       @vulnerability

ee/spec/support/shared_examples/services/vulnerabilities/calls_vulnerability_statistics_utility_services_in_order.rb

@@ -2,14 +2,31 @@
 
 RSpec.shared_examples 'calls vulnerability statistics utility services in order' do
   before do
+    vulnerability.clear_changes_information
+
+    allow(SystemNoteService).to receive(:change_vulnerability_state) { |vulnerability, *| vulnerability.clear_changes_information }
     allow(Vulnerabilities::Statistics::UpdateService).to receive(:update_for)
-    allow(SystemNoteService).to receive(:change_vulnerability_state)
   end
 
-  it 'calls the service classes in order' do
-    subject
+  context 'when updating the vulnerability fails' do
+    before do
+      allow(vulnerability).to receive(:update).and_return(false)
+    end
+
+    it 'does not call the service classes' do
+      subject
+
+      expect(SystemNoteService).not_to have_received(:change_vulnerability_state).ordered
+      expect(Vulnerabilities::Statistics::UpdateService).not_to have_received(:update_for).ordered
+    end
+  end
+
+  context 'when updating the vulnerability succeeds' do
+    it 'calls the service classes in order' do
+      subject
 
-    expect(Vulnerabilities::Statistics::UpdateService).to have_received(:update_for).ordered
-    expect(SystemNoteService).to have_received(:change_vulnerability_state).ordered
+      expect(SystemNoteService).to have_received(:change_vulnerability_state).ordered
+      expect(Vulnerabilities::Statistics::UpdateService).to have_received(:update_for).ordered
+    end
   end
 end