Commit b5bdc55d authored by Tiago Botelho's avatar Tiago Botelho

Move exception handling to execute

parent 0aa8249e
...@@ -15,29 +15,48 @@ module Projects ...@@ -15,29 +15,48 @@ module Projects
def execute def execute
return false unless can?(current_user, :remove_project, project) return false unless can?(current_user, :remove_project, project)
repo_path = project.path_with_namespace
wiki_path = repo_path + '.wiki'
# Flush the cache for both repositories. This has to be done _before_ # Flush the cache for both repositories. This has to be done _before_
# removing the physical repositories as some expiration code depends on # removing the physical repositories as some expiration code depends on
# Git data (e.g. a list of branch names). # Git data (e.g. a list of branch names).
flush_caches(project, wiki_path) flush_caches(project)
Projects::UnlinkForkService.new(project, current_user).execute Projects::UnlinkForkService.new(project, current_user).execute
attempt_destroy_transaction(project, repo_path, wiki_path) attempt_destroy_transaction(project)
system_hook_service.execute_hooks_for(project, :destroy) system_hook_service.execute_hooks_for(project, :destroy)
log_info("Project \"#{project.full_path}\" was removed") log_info("Project \"#{project.full_path}\" was removed")
true true
rescue Projects::DestroyService::DestroyError => error rescue => error
Rails.logger.error("Deletion failed on #{project.full_path} with the following message: #{error.message}") attempt_rollback(project, error.message)
false false
rescue Exception => error # rubocop:disable Lint/RescueException
# Project.transaction can raise Exception
attempt_rollback(project, error.message)
raise
end end
private private
def repo_path
project.path_with_namespace
end
def wiki_path
repo_path + '.wiki'
end
def trash_repositories!
unless remove_repository(repo_path)
raise_error('Failed to remove project repository. Please try again or contact administrator.')
end
unless remove_repository(wiki_path)
raise_error('Failed to remove wiki repository. Please try again or contact administrator.')
end
end
def remove_repository(path) def remove_repository(path)
# Skip repository removal. We use this flag when remove user or group # Skip repository removal. We use this flag when remove user or group
return true if params[:skip_repo] == true return true if params[:skip_repo] == true
...@@ -59,26 +78,24 @@ module Projects ...@@ -59,26 +78,24 @@ module Projects
end end
end end
def attempt_destroy_transaction(project, repo_path, wiki_path) def attempt_rollback(project, message)
return unless project
project.update_attributes(delete_error: message, pending_delete: false)
log_error("Deletion failed on #{project.full_path} with the following message: #{message}")
end
def attempt_destroy_transaction(project)
Project.transaction do Project.transaction do
unless remove_legacy_registry_tags unless remove_legacy_registry_tags
raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.') raise_error('Failed to remove some tags in project container registry. Please try again or contact administrator.')
end end
unless remove_repository(repo_path) trash_repositories!
raise_error('Failed to remove project repository. Please try again or contact administrator.')
end
unless remove_repository(wiki_path)
raise_error('Failed to remove wiki repository. Please try again or contact administrator.')
end
project.team.truncate project.team.truncate
project.destroy! project.destroy!
end end
rescue Exception => error # rubocop:disable Lint/RescueException
project.update_attributes(delete_error: error.message, pending_delete: false)
raise
end end
## ##
...@@ -107,7 +124,7 @@ module Projects ...@@ -107,7 +124,7 @@ module Projects
"#{path}+#{project.id}#{DELETED_FLAG}" "#{path}+#{project.id}#{DELETED_FLAG}"
end end
def flush_caches(project, wiki_path) def flush_caches(project)
project.repository.before_delete project.repository.before_delete
Repository.new(wiki_path, project).before_delete Repository.new(wiki_path, project).before_delete
......
- if @project.delete_error.present? - project = local_assigns.fetch(:project)
.project-deletion-failed-message.alert.alert-warning - return unless project.delete_error.present?
This project was scheduled for deletion, but failed with the following message:
= @project.delete_error
.alert-link-group .project-deletion-failed-message.alert.alert-warning
= link_to "Don't show again", profile_path(user: { hide_no_ssh_key: true }), method: :put, class: 'alert-link' This project was scheduled for deletion, but failed with the following message:
| = project.delete_error
= link_to 'Remind later', '#', class: 'hide-no-ssh-message alert-link'
- project = local_assigns.fetch(:project)
- flash_message_container = show_new_nav? ? :new_global_flash : :flash_message
= content_for flash_message_container do = content_for flash_message_container do
= render 'deletion_failed' = render partial: 'deletion_failed', locals: { project: project }
- if current_user && can?(current_user, :download_code, project) - if current_user && can?(current_user, :download_code, project)
= render 'shared/no_ssh' = render 'shared/no_ssh'
= render 'shared/no_password' = render 'shared/no_password'
- @no_container = true - @no_container = true
- flash_message_container = show_new_nav? ? :new_global_flash : :flash_message
= render partial: 'flash_messages', locals: { project: @project } = render partial: 'flash_messages', locals: { project: @project }
......
- @no_container = true - @no_container = true
- breadcrumb_title "Project" - breadcrumb_title "Project"
- @content_class = "limit-container-width" unless fluid_layout - @content_class = "limit-container-width" unless fluid_layout
- flash_message_container = show_new_nav? ? :new_global_flash : :flash_message
= content_for :meta_tags do = content_for :meta_tags do
= auto_discovery_link_tag(:atom, project_path(@project, rss_url_options), title: "#{@project.name} activity") = auto_discovery_link_tag(:atom, project_path(@project, rss_url_options), title: "#{@project.name} activity")
......
...@@ -8,6 +8,6 @@ class ProjectDestroyWorker ...@@ -8,6 +8,6 @@ class ProjectDestroyWorker
::Projects::DestroyService.new(project, user, params.symbolize_keys).execute ::Projects::DestroyService.new(project, user, params.symbolize_keys).execute
rescue ActiveRecord::RecordNotFound => error rescue ActiveRecord::RecordNotFound => error
logger.error("Failed to delete project #{project.path_with_namespace} (#{project.id}): #{error.message}") logger.error("Failed to delete project (#{project_id}): #{error.message}")
end end
end end
---
title: Handle errors while a project is being deleted asynchronously.
merge_request: 11088
author:
...@@ -3,28 +3,18 @@ require 'spec_helper' ...@@ -3,28 +3,18 @@ require 'spec_helper'
describe 'Project show page', feature: true do describe 'Project show page', feature: true do
context 'when project pending delete' do context 'when project pending delete' do
let(:project) { create(:project, :empty_repo, pending_delete: true) } let(:project) { create(:project, :empty_repo, pending_delete: true) }
let(:worker) { ProjectDestroyWorker.new }
before do before do
sign_in(project.owner) sign_in(project.owner)
end end
it 'shows flash error if deletion for project fails' do it 'shows error message if deletion for project fails' do
error_message = "some error message" project.update_attributes(delete_error: "Something went wrong", pending_delete: false)
project.update_attributes(delete_error: error_message, pending_delete: false)
visit namespace_project_path(project.namespace, project) visit project_path(project)
expect(page).to have_selector('.project-deletion-failed-message') expect(page).to have_selector('.project-deletion-failed-message')
expect(page).to have_content("This project was scheduled for deletion, but failed with the following message: #{error_message}") expect(page).to have_content("This project was scheduled for deletion, but failed with the following message: #{project.delete_error}")
end
it 'renders 404 if project was successfully deleted' do
worker.perform(project.id, project.owner.id, {})
visit namespace_project_path(project.namespace, project)
expect(page).to have_http_status(404)
end end
end end
end end
...@@ -130,30 +130,29 @@ describe Projects::DestroyService, services: true do ...@@ -130,30 +130,29 @@ describe Projects::DestroyService, services: true do
it_behaves_like 'handles errors thrown during async destroy', "Failed to remove project repository" it_behaves_like 'handles errors thrown during async destroy', "Failed to remove project repository"
end end
context 'when `execute` raises any other error' do context 'when `execute` raises expected error' do
before do before do
expect_any_instance_of(Projects::DestroyService) expect_any_instance_of(Project)
.to receive(:execute).and_raise(ArgumentError.new("Other error message")) .to receive(:destroy!).and_raise(StandardError.new("Other error message"))
end end
it_behaves_like 'handles errors thrown during async destroy', "Other error message" it_behaves_like 'handles errors thrown during async destroy', "Other error message"
end end
end
end
context 'with execute' do
it_behaves_like 'deleting the project with pipeline and build'
context 'when `execute` raises an error' do context 'when `execute` raises unexpected error' do
before do before do
expect_any_instance_of(Projects::DestroyService) expect_any_instance_of(Project)
.to receive(:execute).and_raise(ArgumentError) .to receive(:destroy!).and_raise(Exception.new("Other error message"))
end end
it 'allows the error to bubble up' do it 'allows error to bubble up and rolls back project deletion' do
expect do expect do
Sidekiq::Testing.inline! { Projects::DestroyService.new(project, user, {}).execute } Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
end.to raise_error(ArgumentError) end.to raise_error
expect(project.reload.pending_delete).to be(false)
expect(project.delete_error).to include("Other error message")
end
end end
end end
end end
...@@ -182,8 +181,7 @@ describe Projects::DestroyService, services: true do ...@@ -182,8 +181,7 @@ describe Projects::DestroyService, services: true do
expect_any_instance_of(ContainerRepository) expect_any_instance_of(ContainerRepository)
.to receive(:delete_tags!).and_return(false) .to receive(:delete_tags!).and_return(false)
expect{ destroy_project(project, user) } expect(destroy_project(project, user)).to be false
.to raise_error(ActiveRecord::RecordNotDestroyed)
end end
end end
end end
...@@ -208,8 +206,7 @@ describe Projects::DestroyService, services: true do ...@@ -208,8 +206,7 @@ describe Projects::DestroyService, services: true do
expect_any_instance_of(ContainerRepository) expect_any_instance_of(ContainerRepository)
.to receive(:delete_tags!).and_return(false) .to receive(:delete_tags!).and_return(false)
expect { destroy_project(project, user) } expect(destroy_project(project, user)).to be false
.to raise_error(Projects::DestroyService::DestroyError)
end end
end end
end end
......
...@@ -21,17 +21,16 @@ describe ProjectDestroyWorker do ...@@ -21,17 +21,16 @@ describe ProjectDestroyWorker do
expect(Dir.exist?(path)).to be_truthy expect(Dir.exist?(path)).to be_truthy
end end
describe 'when StandardError is raised' do it 'does not raise error when project could not be found' do
it 'reverts pending_delete attribute with a error message' do
allow_any_instance_of(::Projects::DestroyService).to receive(:execute).and_raise(StandardError, "some error message")
expect do expect do
subject.perform(project.id, project.owner.id, {}) subject.perform(-1, project.owner.id, {})
end.to change { project.reload.pending_delete }.from(true).to(false) end.not_to raise_error
expect(Project.all).to include(project)
expect(project.delete_error).to eq("some error message")
end end
it 'does not raise error when user could not be found' do
expect do
subject.perform(project.id, -1, {})
end.not_to raise_error
end end
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