Merge branch '336837-persist-webhook-disabled-state-when-records-are-invalid' into 'master'

Ensure WebHook rate-limiting is persisted if record is invalid

See merge request gitlab-org/gitlab!66876

app/models/hooks/web_hook.rb

@@ -69,21 +69,26 @@ class WebHook < ApplicationRecord
   end
 
   def disable!
-    update!(recent_failures: FAILURE_THRESHOLD + 1)
+    update_attribute(:recent_failures, FAILURE_THRESHOLD + 1)
   end
 
   def enable!
     return if recent_failures == 0 && disabled_until.nil? && backoff_count == 0
 
-    update!(recent_failures: 0, disabled_until: nil, backoff_count: 0)
+    assign_attributes(recent_failures: 0, disabled_until: nil, backoff_count: 0)
+    save(validate: false)
   end
 
   def backoff!
-    update!(disabled_until: next_backoff.from_now, backoff_count: backoff_count.succ.clamp(0, MAX_FAILURES))
+    assign_attributes(disabled_until: next_backoff.from_now, backoff_count: backoff_count.succ.clamp(0, MAX_FAILURES))
+    save(validate: false)
   end
 
   def failed!
-    update!(recent_failures: recent_failures + 1) if recent_failures < MAX_FAILURES
+    return unless recent_failures < MAX_FAILURES
+
+    assign_attributes(recent_failures: recent_failures + 1)
+    save(validate: false)
   end
 
   # Overridden in ProjectHook and GroupHook, other webhooks are not rate-limited.

spec/models/hooks/web_hook_spec.rb

@@ -267,6 +267,15 @@ RSpec.describe WebHook do
     end
   end
 
+  shared_examples 'is tolerant of invalid records' do
+    specify do
+      hook.url = nil
+
+      expect(hook).to be_invalid
+      run_expectation
+    end
+  end
+
   describe '#enable!' do
     it 'makes a hook executable if it was marked as failed' do
       hook.recent_failures = 1000
@@ -280,16 +289,18 @@ RSpec.describe WebHook do
       expect { hook.enable! }.to change(hook, :executable?).from(false).to(true)
     end
 
-    it 'does not update hooks unless necessary' do
-      expect(hook).not_to receive(:update!)
+    it 'does not update hooks unless necessary', :aggregate_failures do
+      sql_count = ActiveRecord::QueryRecorder.new { hook.enable! }.count
 
-      hook.enable!
+      expect(sql_count).to eq(0)
     end
 
-    it 'is idempotent on executable hooks' do
-      expect(hook).not_to receive(:update!)
+    include_examples 'is tolerant of invalid records' do
+      def run_expectation
+        hook.recent_failures = 1000
 
-      expect { hook.enable! }.not_to change(hook, :executable?)
+        expect { hook.enable! }.to change(hook, :executable?).from(false).to(true)
+      end
     end
   end
 
@@ -307,6 +318,12 @@ RSpec.describe WebHook do
 
       expect { hook.backoff! }.not_to change(hook, :backoff_count)
     end
+
+    include_examples 'is tolerant of invalid records' do
+      def run_expectation
+        expect { hook.backoff! }.to change(hook, :backoff_count).by(1)
+      end
+    end
   end
 
   describe 'failed!' do
@@ -314,11 +331,17 @@ RSpec.describe WebHook do
       expect { hook.failed! }.to change(hook, :recent_failures).by(1)
     end
 
-    it 'does not allow the failure count to exceed the maximum value' do
+    it 'does not allow the failure count to exceed the maximum value', :aggregate_failures do
       hook.recent_failures = described_class::MAX_FAILURES
-      expect(hook).not_to receive(:update!)
 
       expect { hook.failed! }.not_to change(hook, :recent_failures)
+      expect(hook).not_to be_persisted
+    end
+
+    include_examples 'is tolerant of invalid records' do
+      def run_expectation
+        expect { hook.failed! }.to change(hook, :recent_failures).by(1)
+      end
     end
   end
 
@@ -326,5 +349,11 @@ RSpec.describe WebHook do
     it 'disables a hook' do
       expect { hook.disable! }.to change(hook, :executable?).from(true).to(false)
     end
+
+    include_examples 'is tolerant of invalid records' do
+      def run_expectation
+        expect { hook.disable! }.to change(hook, :executable?).from(true).to(false)
+      end
+    end
   end
 end