Commit c7680264 authored by Yorick Peterse's avatar Yorick Peterse

Merge branch '30306-transaction-while-moving-issues-to-ghost-user' into 'master'

Add a transaction around `move_issues_to_ghost_user`

See merge request !10465
parents e9f50438 cea8342b
......@@ -15,27 +15,39 @@ module Users
end
def execute
# 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
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
# Reverse the user block if record migration fails
if !migrate_records && transition
transition.rollback
user.save!
end
end
user.reload
end
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_emoji
migrate_award_emojis
end
user.reload
end
private
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
......
---
title: Add a transaction around move_issues_to_ghost_user
merge_request: 10465
author:
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment