Move records to the ghost user in a transaction.
- While deleting a user, some of the user's associated records are moved to the ghost user so they aren't deleted. The user is blocked before these records are moved, to prevent the user from creating new records while the migration is happening, and so preventing a data race. - Previously, if the migration failed, the user would _remain_ blocked, which is not the expected behavior. On the other hand, we can't just stick the block + migration into a transaction, because we want the block to be committed before the migration starts (for the data race reason mentioned above). - One solution (implemented in this commit) is to block the user in a parent transaction, migrate the associated records in a nested sub-transaction, and then unblock the user in the parent transaction if the sub-transaction fails.
This commit is contained in:
parent
6c65b63ca5
commit
133f00bedd
|
@ -15,20 +15,20 @@ module Users
|
|||
end
|
||||
|
||||
def execute
|
||||
transition = user.block_transition
|
||||
|
||||
user.transaction do
|
||||
# Block the user before moving records to prevent a data race.
|
||||
# For example, if the user creates an issue after `migrate_issues`
|
||||
# runs and before the user is destroyed, the destroy will fail with
|
||||
# an exception.
|
||||
user.block
|
||||
|
||||
user.transaction do
|
||||
@ghost_user = User.ghost
|
||||
|
||||
migrate_issues
|
||||
migrate_merge_requests
|
||||
migrate_notes
|
||||
migrate_abuse_reports
|
||||
migrate_award_emoji
|
||||
# Reverse the user block if record migration fails
|
||||
if !migrate_records && transition
|
||||
transition.rollback
|
||||
user.save!
|
||||
end
|
||||
end
|
||||
|
||||
user.reload
|
||||
|
@ -36,6 +36,18 @@ module Users
|
|||
|
||||
private
|
||||
|
||||
def migrate_records
|
||||
user.transaction(requires_new: true) do
|
||||
@ghost_user = User.ghost
|
||||
|
||||
migrate_issues
|
||||
migrate_merge_requests
|
||||
migrate_notes
|
||||
migrate_abuse_reports
|
||||
migrate_award_emojis
|
||||
end
|
||||
end
|
||||
|
||||
def migrate_issues
|
||||
user.issues.update_all(author_id: ghost_user.id)
|
||||
end
|
||||
|
@ -52,7 +64,7 @@ module Users
|
|||
user.reported_abuse_reports.update_all(reporter_id: ghost_user.id)
|
||||
end
|
||||
|
||||
def migrate_award_emoji
|
||||
def migrate_award_emojis
|
||||
user.award_emoji.update_all(user_id: ghost_user.id)
|
||||
end
|
||||
end
|
||||
|
|
|
@ -60,5 +60,23 @@ describe Users::MigrateToGhostUserService, services: true do
|
|||
end
|
||||
end
|
||||
end
|
||||
|
||||
context "when record migration fails with a rollback exception" do
|
||||
before do
|
||||
expect_any_instance_of(MergeRequest::ActiveRecord_Associations_CollectionProxy)
|
||||
.to receive(:update_all).and_raise(ActiveRecord::Rollback)
|
||||
end
|
||||
|
||||
context "for records that were already migrated" do
|
||||
let!(:issue) { create(:issue, project: project, author: user) }
|
||||
let!(:merge_request) { create(:merge_request, source_project: project, author: user, target_branch: "first") }
|
||||
|
||||
it "reverses the migration" do
|
||||
service.execute
|
||||
|
||||
expect(issue.reload.author).to eq(user)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -35,5 +35,57 @@ shared_examples "migrating a deleted user's associated records to the ghost user
|
|||
|
||||
expect(user).to be_blocked
|
||||
end
|
||||
|
||||
context "race conditions" do
|
||||
context "when #{record_class_name} migration fails and is rolled back" do
|
||||
before do
|
||||
expect_any_instance_of(record_class::ActiveRecord_Associations_CollectionProxy)
|
||||
.to receive(:update_all).and_raise(ActiveRecord::Rollback)
|
||||
end
|
||||
|
||||
it 'rolls back the user block' do
|
||||
service.execute
|
||||
|
||||
expect(user.reload).not_to be_blocked
|
||||
end
|
||||
|
||||
it "doesn't unblock an previously-blocked user" do
|
||||
user.block
|
||||
|
||||
service.execute
|
||||
|
||||
expect(user.reload).to be_blocked
|
||||
end
|
||||
end
|
||||
|
||||
context "when #{record_class_name} migration fails with a non-rollback exception" do
|
||||
before do
|
||||
expect_any_instance_of(record_class::ActiveRecord_Associations_CollectionProxy)
|
||||
.to receive(:update_all).and_raise(ArgumentError)
|
||||
end
|
||||
|
||||
it 'rolls back the user block' do
|
||||
service.execute rescue nil
|
||||
|
||||
expect(user.reload).not_to be_blocked
|
||||
end
|
||||
|
||||
it "doesn't unblock an previously-blocked user" do
|
||||
user.block
|
||||
|
||||
service.execute rescue nil
|
||||
|
||||
expect(user.reload).to be_blocked
|
||||
end
|
||||
end
|
||||
|
||||
it "blocks the user before #{record_class_name} migration begins" do
|
||||
expect(service).to receive("migrate_#{record_class_name.parameterize('_')}s".to_sym) do
|
||||
expect(user.reload).to be_blocked
|
||||
end
|
||||
|
||||
service.execute
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
Loading…
Reference in New Issue