Merge branch '337336-user-deletion-times-out-due-to-merge-request-update' into 'master'

Nullify dependent associations in batches on user deletion

See merge request gitlab-org/gitlab!84709

app/models/concerns/batch_nullify_dependent_associations.rb

+# frozen_string_literal: true
+
+# Provides a way to execute nullify behaviour in batches
+# to avoid query timeouts for really big tables
+# Assumes that associations have `dependent: :nullify` statement
+module BatchNullifyDependentAssociations
+  extend ActiveSupport::Concern
+
+  class_methods do
+    def dependent_associations_to_nullify
+      reflect_on_all_associations(:has_many).select { |assoc| assoc.options[:dependent] == :nullify }
+    end
+  end
+
+  def nullify_dependent_associations_in_batches(exclude: [], batch_size: 100)
+    self.class.dependent_associations_to_nullify.each do |association|
+      next if association.name.in?(exclude)
+
+      loop do
+        # rubocop:disable GitlabSecurity/PublicSend
+        update_count = public_send(association.name).limit(batch_size).update_all(association.foreign_key => nil)
+        # rubocop:enable GitlabSecurity/PublicSend
+        break if update_count < batch_size
+      end
+    end
+  end
+end

app/models/user.rb

@@ -21,6 +21,7 @@ class User < ApplicationRecord
   include OptionallySearch
   include FromUnion
   include BatchDestroyDependentAssociations
+  include BatchNullifyDependentAssociations
   include HasUniqueInternalUsers
   include IgnorableColumns
   include UpdateHighestRole
@@ -189,6 +190,8 @@ class User < ApplicationRecord
   has_many :snippets,                 dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent
   has_many :notes,                    dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent
   has_many :issues,                   dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent
+  has_many :updated_issues, class_name: 'Issue', dependent: :nullify, foreign_key: :updated_by_id # rubocop:disable Cop/ActiveRecordDependent
+  has_many :closed_issues, class_name: 'Issue', dependent: :nullify, foreign_key: :closed_by_id # rubocop:disable Cop/ActiveRecordDependent
   has_many :merge_requests,           dependent: :destroy, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent
   has_many :events,                   dependent: :delete_all, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent
   has_many :releases,                 dependent: :nullify, foreign_key: :author_id # rubocop:disable Cop/ActiveRecordDependent

app/services/users/destroy_service.rb

@@ -64,6 +64,10 @@ module Users
       # This ensures we delete records in batches.
       user.destroy_dependent_associations_in_batches(exclude: [:snippets])
 
+      if Feature.enabled?(:nullify_in_batches_on_user_deletion, default_enabled: :yaml)
+        user.nullify_dependent_associations_in_batches
+      end
+
       # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing
       user_data = user.destroy
       namespace.destroy

app/services/users/migrate_to_ghost_user_service.rb

@@ -100,9 +100,9 @@ module Users
     end
 
     # rubocop:disable CodeReuse/ActiveRecord
-    def batched_migrate(base_scope, column)
+    def batched_migrate(base_scope, column, batch_size: 50)
       loop do
-        update_count = base_scope.where(column => user.id).limit(100).update_all(column => ghost_user.id)
+        update_count = base_scope.where(column => user.id).limit(batch_size).update_all(column => ghost_user.id)
         break if update_count == 0
       end
     end

config/feature_flags/development/nullify_in_batches_on_user_deletion.yml

+---
+name: 'nullify_in_batches_on_user_deletion'
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/84709
+rollout_issue_url:
+milestone: '14.10'
+type: development
+group: group::optimize
+default_enabled: true

spec/models/concerns/batch_nullify_dependent_associations_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe BatchNullifyDependentAssociations do
+  before do
+    test_user = Class.new(ActiveRecord::Base) do
+      self.table_name = 'users'
+
+      has_many :closed_issues, foreign_key: :closed_by_id, class_name: 'Issue', dependent: :nullify
+      has_many :issues, foreign_key: :author_id, class_name: 'Issue', dependent: :nullify
+      has_many :updated_issues, foreign_key: :updated_by_id, class_name: 'Issue'
+
+      include BatchNullifyDependentAssociations
+    end
+
+    stub_const('TestUser', test_user)
+  end
+
+  describe '.dependent_associations_to_nullify' do
+    it 'returns only associations with `dependent: :nullify` associations' do
+      expect(TestUser.dependent_associations_to_nullify.map(&:name)).to match_array([:closed_issues, :issues])
+    end
+  end
+
+  describe '#nullify_dependent_associations_in_batches' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:updated_by_issue) { create(:issue, updated_by: user) }
+
+    before do
+      create(:issue, closed_by: user)
+      create(:issue, closed_by: user)
+    end
+
+    it 'nullifies multiple settings' do
+      expect do
+        test_user = TestUser.find(user.id)
+        test_user.nullify_dependent_associations_in_batches
+      end.to change { Issue.where(closed_by_id: user.id).count }.by(-2)
+    end
+
+    it 'excludes associations' do
+      expect do
+        test_user = TestUser.find(user.id)
+        test_user.nullify_dependent_associations_in_batches(exclude: [:closed_issues])
+      end.not_to change { Issue.where(closed_by_id: user.id).count }
+    end
+  end
+end

spec/services/users/destroy_service_spec.rb

@@ -332,4 +332,39 @@ RSpec.describe Users::DestroyService do
       expect(User.exists?(other_user.id)).to be(false)
     end
   end
+
+  context 'batched nullify' do
+    let(:other_user) { create(:user) }
+
+    context 'when :nullify_in_batches_on_user_deletion feature flag is enabled' do
+      it 'nullifies related associations in batches' do
+        expect(other_user).to receive(:nullify_dependent_associations_in_batches).and_call_original
+
+        described_class.new(user).execute(other_user, skip_authorization: true)
+      end
+
+      it 'nullifies last_updated_issues and closed_issues' do
+        issue = create(:issue, closed_by: other_user, updated_by: other_user)
+
+        described_class.new(user).execute(other_user, skip_authorization: true)
+
+        issue.reload
+
+        expect(issue.closed_by).to be_nil
+        expect(issue.updated_by).to be_nil
+      end
+    end
+
+    context 'when :nullify_in_batches_on_user_deletion feature flag is disabled' do
+      before do
+        stub_feature_flags(nullify_in_batches_on_user_deletion: false)
+      end
+
+      it 'does not use batching' do
+        expect(other_user).not_to receive(:nullify_dependent_associations_in_batches)
+
+        described_class.new(user).execute(other_user, skip_authorization: true)
+      end
+    end
+  end
 end