Merge branch '30213-project-transfer-move-rollback' into 'master'

Rollback project folder move after error in Projects::TransferService

Closes #30213

See merge request !11877

app/services/projects/transfer_service.rb

@@ -12,87 +12,121 @@ module Projects
     TransferError = Class.new(StandardError)
 
     def execute(new_namespace)
-      if new_namespace.blank?
+      @new_namespace = new_namespace
+
+      if @new_namespace.blank?
         raise TransferError, 'Please select a new namespace for your project.'
       end
-      unless allowed_transfer?(current_user, project, new_namespace)
+
+      unless allowed_transfer?(current_user, project)
         raise TransferError, 'Transfer failed, please contact an admin.'
       end
-      transfer(project, new_namespace)
+
+      transfer(project)
+
+      true
     rescue Projects::TransferService::TransferError => ex
       project.reload
       project.errors.add(:new_namespace, ex.message)
       false
     end
 
-    def transfer(project, new_namespace)
-      old_namespace = project.namespace
+    private
 
-      Project.transaction do
-        old_path = project.path_with_namespace
-        old_group = project.group
-        new_path = File.join(new_namespace.try(:full_path) || '', project.path)
+    def transfer(project)
+      @old_path = project.path_with_namespace
+      @old_group = project.group
+      @new_path = File.join(@new_namespace.try(:full_path) || '', project.path)
+      @old_namespace = project.namespace
 
-        if Project.where(path: project.path, namespace_id: new_namespace.try(:id)).present?
+      if Project.where(path: project.path, namespace_id: @new_namespace.try(:id)).exists?
         raise TransferError.new("Project with same path in target namespace already exists")
       end
 
       if project.has_container_registry_tags?
-          # we currently doesn't support renaming repository if it contains tags in container registry
+        # We currently don't support renaming repository if it contains tags in container registry
         raise TransferError.new('Project cannot be transferred, because tags are present in its container registry')
       end
 
-        project.expire_caches_before_rename(old_path)
+      attempt_transfer_transaction
+    end
+
+    def attempt_transfer_transaction
+      Project.transaction do
+        project.expire_caches_before_rename(@old_path)
 
-        # Apply new namespace id and visibility level
-        project.namespace = new_namespace
-        project.visibility_level = new_namespace.visibility_level unless project.visibility_level_allowed_by_group?
-        project.save!
+        update_namespace_and_visibility(@new_namespace)
 
         # Notifications
-        project.send_move_instructions(old_path)
+        project.send_move_instructions(@old_path)
 
         # Move main repository
-        unless gitlab_shell.mv_repository(project.repository_storage_path, old_path, new_path)
+        unless move_repo_folder(@old_path, @new_path)
           raise TransferError.new('Cannot move project')
         end
 
         # Move wiki repo also if present
-        gitlab_shell.mv_repository(project.repository_storage_path, "#{old_path}.wiki", "#{new_path}.wiki")
+        move_repo_folder("#{@old_path}.wiki", "#{@new_path}.wiki")
 
         # Move missing group labels to project
-        Labels::TransferService.new(current_user, old_group, project).execute
+        Labels::TransferService.new(current_user, @old_group, project).execute
 
         # Move uploads
-        Gitlab::UploadsTransfer.new.move_project(project.path, old_namespace.full_path, new_namespace.full_path)
+        Gitlab::UploadsTransfer.new.move_project(project.path, @old_namespace.full_path, @new_namespace.full_path)
 
         # Move pages
-        Gitlab::PagesTransfer.new.move_project(project.path, old_namespace.full_path, new_namespace.full_path)
+        Gitlab::PagesTransfer.new.move_project(project.path, @old_namespace.full_path, @new_namespace.full_path)
 
-        project.old_path_with_namespace = old_path
+        project.old_path_with_namespace = @old_path
 
-        SystemHooksService.new.execute_hooks_for(project, :transfer)
+        execute_system_hooks
       end
-
-      refresh_permissions(old_namespace, new_namespace)
-
-      true
+    rescue Exception # rubocop:disable Lint/RescueException
+      rollback_side_effects
+      raise
+    ensure
+      refresh_permissions
     end
 
-    def allowed_transfer?(current_user, project, namespace)
-      namespace &&
+    def allowed_transfer?(current_user, project)
+      @new_namespace &&
         can?(current_user, :change_namespace, project) &&
-        namespace.id != project.namespace_id &&
-        current_user.can?(:create_projects, namespace)
+        @new_namespace.id != project.namespace_id &&
+        current_user.can?(:create_projects, @new_namespace)
     end
 
-    def refresh_permissions(old_namespace, new_namespace)
+    def update_namespace_and_visibility(to_namespace)
+      # Apply new namespace id and visibility level
+      project.namespace = to_namespace
+      project.visibility_level = to_namespace.visibility_level unless project.visibility_level_allowed_by_group?
+      project.save!
+    end
+
+    def refresh_permissions
       # This ensures we only schedule 1 job for every user that has access to
       # the namespaces.
-      user_ids = old_namespace.user_ids_for_project_authorizations |
-        new_namespace.user_ids_for_project_authorizations
+      user_ids = @old_namespace.user_ids_for_project_authorizations |
+        @new_namespace.user_ids_for_project_authorizations
 
       UserProjectAccessChangedService.new(user_ids).execute
     end
+
+    def rollback_side_effects
+      rollback_folder_move
+      update_namespace_and_visibility(@old_namespace)
+    end
+
+    def rollback_folder_move
+      move_repo_folder(@new_path, @old_path)
+      move_repo_folder("#{@new_path}.wiki", "#{@old_path}.wiki")
+    end
+
+    def move_repo_folder(from_name, to_name)
+      gitlab_shell.mv_repository(project.repository_storage_path, from_name, to_name)
+    end
+
+    def execute_system_hooks
+      SystemHooksService.new.execute_hooks_for(project, :transfer)
+    end
   end
 end

changelogs/unreleased/30213-project-transfer-move-rollback.yml

+---
+title: Rollback project repo move if there is an error in Projects::TransferService
+merge_request: 11877
+author:

spec/services/projects/transfer_service_spec.rb

@@ -19,6 +19,67 @@ describe Projects::TransferService, services: true do
     it { expect(project.namespace).to eq(group) }
   end
 
+  context 'when transfer succeeds' do
+    before do
+      group.add_owner(user)
+    end
+
+    it 'sends notifications' do
+      expect_any_instance_of(NotificationService).to receive(:project_was_moved)
+
+      transfer_project(project, user, group)
+    end
+
+    it 'executes system hooks' do
+      expect_any_instance_of(Projects::TransferService).to receive(:execute_system_hooks)
+
+      transfer_project(project, user, group)
+    end
+  end
+
+  context 'when transfer fails' do
+    let!(:original_path) { project_path(project) }
+
+    def attempt_project_transfer
+      expect do
+        transfer_project(project, user, group)
+      end.to raise_error(ActiveRecord::ActiveRecordError)
+    end
+
+    before do
+      group.add_owner(user)
+
+      expect_any_instance_of(Labels::TransferService).to receive(:execute).and_raise(ActiveRecord::StatementInvalid, "PG ERROR")
+    end
+
+    def project_path(project)
+      File.join(project.repository_storage_path, "#{project.path_with_namespace}.git")
+    end
+
+    def current_path
+      project_path(project)
+    end
+
+    it 'rolls back repo location' do
+      attempt_project_transfer
+
+      expect(Dir.exist?(original_path)).to be_truthy
+      expect(original_path).to eq current_path
+    end
+
+    it "doesn't send move notifications" do
+      expect_any_instance_of(NotificationService).not_to receive(:project_was_moved)
+
+      attempt_project_transfer
+    end
+
+    it "doesn't run system hooks" do
+      expect_any_instance_of(Projects::TransferService).not_to receive(:execute_system_hooks)
+
+      attempt_project_transfer
+    end
+  end
+
   context 'namespace -> no namespace' do
     before do
       @result = transfer_project(project, user, nil)