summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorYorick Peterse <yorickpeterse@gmail.com>2017-04-24 10:19:18 +0000
committerTimothy Andrew <mail@timothyandrew.net>2017-04-25 05:47:19 +0000
commit7d10817c110278cd18ada01ce8b31bda58cc1abe (patch)
tree60372d88b97d4b6471bf69f14a4bb71f60dcbb97
parent24020e69edce9d6d488eba9a4e27f6db1ac98e38 (diff)
downloadgitlab-ce-7d10817c110278cd18ada01ce8b31bda58cc1abe.tar.gz
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
-rw-r--r--app/services/users/migrate_to_ghost_user_service.rb34
-rw-r--r--changelogs/unreleased/30306-transaction-while-moving-issues-to-ghost-user.yml4
-rw-r--r--spec/services/users/migrate_to_ghost_user_service_spec.rb18
-rw-r--r--spec/support/services/migrate_to_ghost_user_service_shared_examples.rb52
4 files changed, 97 insertions, 11 deletions
diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb
index 1e1ed1791ec..4628c4c6f6e 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/changelogs/unreleased/30306-transaction-while-moving-issues-to-ghost-user.yml b/changelogs/unreleased/30306-transaction-while-moving-issues-to-ghost-user.yml
new file mode 100644
index 00000000000..5fc57e44be6
--- /dev/null
+++ b/changelogs/unreleased/30306-transaction-while-moving-issues-to-ghost-user.yml
@@ -0,0 +1,4 @@
+---
+title: Add a transaction around move_issues_to_ghost_user
+merge_request: 10465
+author:
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 8c5b7e41c15..9e1edf1ac30 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 0eac587e973..dcc562c684b 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