Merge branch 'fj-refactor-destroy-project-service' into 'master'

Refactor project destroy services

See merge request gitlab-org/gitlab!22908

app/services/projects/destroy_rollback_service.rb

+# frozen_string_literal: true
+
+module Projects
+  class DestroyRollbackService < BaseService
+    include Gitlab::ShellAdapter
+
+    def execute
+      return unless project
+
+      Projects::ForksCountService.new(project).delete_cache
+
+      unless rollback_repository(project.repository)
+        raise_error(s_('DeleteProject|Failed to restore project repository. Please contact the administrator.'))
+      end
+
+      unless rollback_repository(project.wiki.repository)
+        raise_error(s_('DeleteProject|Failed to restore wiki repository. Please contact the administrator.'))
+      end
+    end
+
+    private
+
+    def rollback_repository(repository)
+      return true unless repository
+
+      result = Repositories::DestroyRollbackService.new(repository).execute
+
+      result[:status] == :success
+    end
+  end
+end

app/services/projects/destroy_service.rb

@@ -6,9 +6,6 @@ module Projects
 
     DestroyError = Class.new(StandardError)
 
-    DELETED_FLAG = '+deleted'
-    REPO_REMOVAL_DELAY = 5.minutes.to_i
-
     def async_execute
       project.update_attribute(:pending_delete, true)
 
@@ -18,7 +15,7 @@ module Projects
       schedule_stale_repos_removal
 
       job_id = ProjectDestroyWorker.perform_async(project.id, current_user.id, params)
-      Rails.logger.info("User #{current_user.id} scheduled destruction of project #{project.full_path} with job ID #{job_id}") # rubocop:disable Gitlab/RailsLogger
+      log_info("User #{current_user.id} scheduled destruction of project #{project.full_path} with job ID #{job_id}")
     end
 
     def execute
@@ -48,82 +45,34 @@ module Projects
       raise
     end
 
-    def attempt_repositories_rollback
-      return unless @project
-
-      flush_caches(@project)
-
-      unless rollback_repository(removal_path(repo_path), repo_path)
-        raise_error(s_('DeleteProject|Failed to restore project repository. Please contact the administrator.'))
-      end
-
-      unless rollback_repository(removal_path(wiki_path), wiki_path)
-        raise_error(s_('DeleteProject|Failed to restore wiki repository. Please contact the administrator.'))
-      end
-    end
-
     private
 
-    def repo_path
-      project.disk_path
-    end
-
-    def wiki_path
-      project.wiki.disk_path
-    end
-
     def trash_repositories!
-      unless remove_repository(repo_path)
+      unless remove_repository(project.repository)
         raise_error(s_('DeleteProject|Failed to remove project repository. Please try again or contact administrator.'))
       end
 
-      unless remove_repository(wiki_path)
+      unless remove_repository(project.wiki.repository)
         raise_error(s_('DeleteProject|Failed to remove wiki repository. Please try again or contact administrator.'))
       end
     end
 
-    def remove_repository(path)
-      # There is a possibility project does not have repository or wiki
-      return true unless repo_exists?(path)
+    def remove_repository(repository)
+      return true unless repository
 
-      new_path = removal_path(path)
+      result = Repositories::DestroyService.new(repository).execute
 
-      if mv_repository(path, new_path)
-        log_info(%Q{Repository "#{path}" moved to "#{new_path}" for project "#{project.full_path}"})
-
-        project.run_after_commit do
-          GitlabShellWorker.perform_in(REPO_REMOVAL_DELAY, :remove_repository, self.repository_storage, new_path)
-        end
-      else
-        false
-      end
+      result[:status] == :success
     end
 
     def schedule_stale_repos_removal
-      repo_paths = [removal_path(repo_path), removal_path(wiki_path)]
+      repos = [project.repository, project.wiki.repository]
 
-      # Ideally it should wait until the regular removal phase finishes,
-      # so let's delay it a bit further.
-      repo_paths.each do |path|
-        GitlabShellWorker.perform_in(REPO_REMOVAL_DELAY * 2, :remove_repository, project.repository_storage, path)
-      end
-    end
+      repos.each do |repository|
+        next unless repository
 
-    def rollback_repository(old_path, new_path)
-      # There is a possibility project does not have repository or wiki
-      return true unless repo_exists?(old_path)
-
-      mv_repository(old_path, new_path)
-    end
-
-    def repo_exists?(path)
-      gitlab_shell.repository_exists?(project.repository_storage, path + '.git')
-    end
-
-    def mv_repository(from_path, to_path)
-      return true unless repo_exists?(from_path)
-
-      gitlab_shell.mv_repository(project.repository_storage, from_path, to_path)
+        Repositories::ShellDestroyService.new(repository).execute(Repositories::ShellDestroyService::STALE_REMOVAL_DELAY)
+      end
     end
 
     def attempt_rollback(project, message)
@@ -191,32 +140,9 @@ module Projects
       raise DestroyError.new(message)
     end
 
-    # Build a path for removing repositories
-    # We use `+` because its not allowed by GitLab so user can not create
-    # project with name cookies+119+deleted and capture someone stalled repository
-    #
-    # gitlab/cookies.git -> gitlab/cookies+119+deleted.git
-    #
-    def removal_path(path)
-      "#{path}+#{project.id}#{DELETED_FLAG}"
-    end
-
     def flush_caches(project)
-      ignore_git_errors(repo_path) { project.repository.before_delete }
-
-      ignore_git_errors(wiki_path) { Repository.new(wiki_path, project, disk_path: repo_path).before_delete }
-
       Projects::ForksCountService.new(project).delete_cache
     end
-
-    # If we get a Gitaly error, the repository may be corrupted. We can
-    # ignore these errors since we're going to trash the repositories
-    # anyway.
-    def ignore_git_errors(disk_path, &block)
-      yield
-    rescue Gitlab::Git::CommandError => e
-      Gitlab::GitLogger.warn(class: self.class.name, project_id: project.id, disk_path: disk_path, message: e.to_s)
-    end
   end
 end
 

app/services/projects/overwrite_project_service.rb

@@ -55,7 +55,7 @@ module Projects
     end
 
     def attempt_restore_repositories(project)
-      ::Projects::DestroyService.new(project, @current_user).attempt_repositories_rollback
+      ::Projects::DestroyRollbackService.new(project, @current_user).execute
     end
 
     def add_source_project_to_fork_network(source_project)

app/services/repositories/base_service.rb

+# frozen_string_literal: true
+
+class Repositories::BaseService < BaseService
+  include Gitlab::ShellAdapter
+
+  DELETED_FLAG = '+deleted'
+
+  attr_reader :repository
+
+  delegate :project, :disk_path, :full_path, to: :repository
+  delegate :repository_storage, to: :project
+
+  def initialize(repository)
+    @repository = repository
+  end
+
+  def repo_exists?(path)
+    gitlab_shell.repository_exists?(repository_storage, path + '.git')
+  end
+
+  def mv_repository(from_path, to_path)
+    return true unless repo_exists?(from_path)
+
+    gitlab_shell.mv_repository(repository_storage, from_path, to_path)
+  end
+
+  # Build a path for removing repositories
+  # We use `+` because its not allowed by GitLab so user can not create
+  # project with name cookies+119+deleted and capture someone stalled repository
+  #
+  # gitlab/cookies.git -> gitlab/cookies+119+deleted.git
+  #
+  def removal_path
+    "#{disk_path}+#{project.id}#{DELETED_FLAG}"
+  end
+
+  # If we get a Gitaly error, the repository may be corrupted. We can
+  # ignore these errors since we're going to trash the repositories
+  # anyway.
+  def ignore_git_errors(&block)
+    yield
+  rescue Gitlab::Git::CommandError => e
+    Gitlab::GitLogger.warn(class: self.class.name, project_id: project.id, disk_path: disk_path, message: e.to_s)
+  end
+
+  def move_error(path)
+    error = %Q{Repository "#{path}" could not be moved}
+
+    log_error(error)
+    error(error)
+  end
+end

app/services/repositories/destroy_rollback_service.rb

+# frozen_string_literal: true
+
+class Repositories::DestroyRollbackService < Repositories::BaseService
+  def execute
+    # There is a possibility project does not have repository or wiki
+    return success unless repo_exists?(removal_path)
+
+    # Flush the cache for both repositories.
+    ignore_git_errors { repository.before_delete }
+
+    if mv_repository(removal_path, disk_path)
+      log_info(%Q{Repository "#{removal_path}" moved to "#{disk_path}" for repository "#{full_path}"})
+
+      success
+    else
+      move_error(removal_path)
+    end
+  end
+end

app/services/repositories/destroy_service.rb

+# frozen_string_literal: true
+
+class Repositories::DestroyService < Repositories::BaseService
+  def execute
+    return success unless repository
+    return success unless repo_exists?(disk_path)
+
+    # Flush the cache for both repositories. This has to be done _before_
+    # removing the physical repositories as some expiration code depends on
+    # Git data (e.g. a list of branch names).
+    ignore_git_errors { repository.before_delete }
+
+    if mv_repository(disk_path, removal_path)
+      log_info(%Q{Repository "#{disk_path}" moved to "#{removal_path}" for repository "#{full_path}"})
+
+      current_repository = repository
+      project.run_after_commit do
+        Repositories::ShellDestroyService.new(current_repository).execute
+      end
+
+      log_info("Project \"#{project.full_path}\" was removed")
+
+      success
+    else
+      move_error(disk_path)
+    end
+  end
+end

app/services/repositories/shell_destroy_service.rb

+# frozen_string_literal: true
+
+class Repositories::ShellDestroyService < Repositories::BaseService
+  REPO_REMOVAL_DELAY = 5.minutes.to_i
+  STALE_REMOVAL_DELAY = REPO_REMOVAL_DELAY * 2
+
+  def execute(delay = REPO_REMOVAL_DELAY)
+    return success unless repository
+
+    GitlabShellWorker.perform_in(delay,
+                                 :remove_repository,
+                                 repository_storage,
+                                 removal_path)
+  end
+end

ee/app/services/ee/projects/destroy_service.rb

@@ -34,8 +34,8 @@ module EE
       def log_geo_event(project)
         ::Geo::RepositoryDeletedEventStore.new(
           project,
-          repo_path: repo_path,
-          wiki_path: wiki_path
+          repo_path: project.disk_path,
+          wiki_path: project.wiki.disk_path
         ).create!
       end
 

spec/services/projects/destroy_rollback_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::DestroyRollbackService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:repository) { project.repository }
+  let(:repository_storage) { project.repository_storage }
+
+  subject { described_class.new(project, user, {}).execute }
+
+  describe '#execute' do
+    let(:path) { repository.disk_path + '.git' }
+    let(:removal_path) { "#{repository.disk_path}+#{project.id}#{Repositories::DestroyService::DELETED_FLAG}.git" }
+
+    before do
+      aggregate_failures do
+        expect(TestEnv.storage_dir_exists?(repository_storage, path)).to be_truthy
+        expect(TestEnv.storage_dir_exists?(repository_storage, removal_path)).to be_falsey
+      end
+
+      # Don't run sidekiq to check if renamed repository exists
+      Sidekiq::Testing.fake! { destroy_project(project, user, {}) }
+
+      aggregate_failures do
+        expect(TestEnv.storage_dir_exists?(repository_storage, path)).to be_falsey
+        expect(TestEnv.storage_dir_exists?(repository_storage, removal_path)).to be_truthy
+      end
+    end
+
+    it 'restores the repositories' do
+      Sidekiq::Testing.fake! { subject }
+
+      aggregate_failures do
+        expect(TestEnv.storage_dir_exists?(repository_storage, path)).to be_truthy
+        expect(TestEnv.storage_dir_exists?(repository_storage, removal_path)).to be_falsey
+      end
+    end
+  end
+
+  def destroy_project(project, user, params = {})
+    Projects::DestroyService.new(project, user, params).execute
+  end
+end

spec/services/repositories/destroy_rollback_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Repositories::DestroyRollbackService do
+  let_it_be(:user) { create(:user) }
+  let!(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:repository) { project.repository }
+  let(:path) { repository.disk_path }
+  let(:remove_path) { "#{path}+#{project.id}#{described_class::DELETED_FLAG}" }
+
+  subject { described_class.new(repository).execute }
+
+  before do
+    # Dont run sidekiq to check if renamed repository exists
+    Sidekiq::Testing.fake! { destroy_project(project, user) }
+  end
+
+  it 'moves the repository from the +deleted folder' do
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_truthy
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+
+    subject
+
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy
+  end
+
+  it 'logs the successful action' do
+    expect(Gitlab::AppLogger).to receive(:info)
+
+    subject
+  end
+
+  it 'flushes the repository cache' do
+    expect(repository).to receive(:before_delete)
+
+    subject
+  end
+
+  it 'returns success and does not perform any action if repository path does not exist' do
+    expect(repository).to receive(:disk_path).and_return('foo')
+    expect(repository).not_to receive(:before_delete)
+
+    result = subject
+
+    expect(result[:status]).to eq :success
+  end
+
+  context 'when move operation cannot be performed' do
+    let(:service) { described_class.new(repository) }
+
+    before do
+      allow(service).to receive(:mv_repository).and_return(false)
+    end
+
+    it 'returns error' do
+      result = service.execute
+
+      expect(result[:status]).to eq :error
+    end
+
+    it 'logs the error' do
+      expect(Gitlab::AppLogger).to receive(:error)
+
+      service.execute
+    end
+  end
+
+  def destroy_project(project, user)
+    Projects::DestroyService.new(project, user, {}).execute
+  end
+end

spec/services/repositories/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Repositories::DestroyService do
+  let_it_be(:user) { create(:user) }
+  let!(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:repository) { project.repository }
+  let(:path) { repository.disk_path }
+  let(:remove_path) { "#{path}+#{project.id}#{described_class::DELETED_FLAG}" }
+
+  subject { described_class.new(project.repository).execute }
+
+  it 'moves the repository to a +deleted folder' do
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_truthy
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+
+    subject
+
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_truthy
+  end
+
+  it 'schedules the repository deletion' do
+    subject
+
+    expect(Repositories::ShellDestroyService).to receive(:new).with(repository).and_call_original
+
+    expect(GitlabShellWorker).to receive(:perform_in)
+      .with(Repositories::ShellDestroyService::REPO_REMOVAL_DELAY, :remove_repository, project.repository_storage, remove_path)
+
+    # Because GitlabShellWorker is inside a run_after_commit callback we need to
+    # trigger the callback
+    project.touch
+  end
+
+  it 'removes the repository', :sidekiq_inline do
+    subject
+
+    project.touch
+
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, path + '.git')).to be_falsey
+    expect(project.gitlab_shell.repository_exists?(project.repository_storage, remove_path + '.git')).to be_falsey
+  end
+
+  it 'flushes the repository cache' do
+    expect(repository).to receive(:before_delete)
+
+    subject
+  end
+
+  it 'does not perform any action if repository path does not exist and returns success' do
+    expect(repository).to receive(:disk_path).and_return('foo')
+    expect(repository).not_to receive(:before_delete)
+
+    result = subject
+
+    expect(result[:status]).to eq :success
+  end
+
+  context 'when move operation cannot be performed' do
+    let(:service) { described_class.new(repository) }
+
+    before do
+      allow(service).to receive(:mv_repository).and_return(false)
+    end
+
+    it 'returns error' do
+      result = service.execute
+
+      expect(result[:status]).to eq :error
+    end
+
+    it 'logs the error' do
+      expect(Gitlab::AppLogger).to receive(:error)
+
+      service.execute
+    end
+  end
+end

spec/services/repositories/shell_destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Repositories::ShellDestroyService do
+  let_it_be(:user) { create(:user) }
+  let!(:project) { create(:project, :repository, namespace: user.namespace) }
+  let(:path) { project.repository.disk_path }
+  let(:remove_path) { "#{path}+#{project.id}#{described_class::DELETED_FLAG}" }
+
+  it 'returns success if the repository is nil' do
+    expect(GitlabShellWorker).not_to receive(:perform_in)
+
+    result = described_class.new(nil).execute
+
+    expect(result[:status]).to eq :success
+  end
+
+  it 'schedules the repository deletion' do
+    expect(GitlabShellWorker).to receive(:perform_in)
+      .with(described_class::REPO_REMOVAL_DELAY, :remove_repository, project.repository_storage, remove_path)
+
+    described_class.new(project.repository).execute
+  end
+end