diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb index 1e1ed1791ec7e90397ff5a9a1a7bfdcc724d3bc3..4628c4c6f6e7d9b1db442b1a59fdaf5c480260c0 100644 --- a/app/services/users/migrate_to_ghost_user_service.rb +++ b/app/services/users/migrate_to_ghost_user_service.rb @@ -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 diff --git a/spec/services/users/migrate_to_ghost_user_service_spec.rb b/spec/services/users/migrate_to_ghost_user_service_spec.rb index 8c5b7e41c15502edd3417a5ebd40afe565768856..9e1edf1ac30d89e3a5605958a5cfe2400e19947c 100644 --- a/spec/services/users/migrate_to_ghost_user_service_spec.rb +++ b/spec/services/users/migrate_to_ghost_user_service_spec.rb @@ -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 diff --git a/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb index 0eac587e9730a56a40744ae256451b22076dd1c2..dcc562c684b0f4aec57ff0ea46566b8a2915836a 100644 --- a/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb +++ b/spec/support/services/migrate_to_ghost_user_service_shared_examples.rb @@ -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