diff options
| -rw-r--r-- | app/models/namespace.rb | 6 | ||||
| -rw-r--r-- | app/services/groups/destroy_service.rb | 3 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-fix-project-destroy-in-namespace.yml | 4 | ||||
| -rw-r--r-- | spec/models/namespace_spec.rb | 11 | ||||
| -rw-r--r-- | spec/services/groups/destroy_service_spec.rb | 52 |
5 files changed, 54 insertions, 22 deletions
diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 743e0513e02..672eab94c07 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -219,6 +219,12 @@ class Namespace < ActiveRecord::Base parent.present? end + def soft_delete_without_removing_associations + # We can't use paranoia's `#destroy` since this will hard-delete projects. + # Project uses `pending_delete` instead of the acts_as_paranoia gem. + self.deleted_at = Time.now + end + private def repository_storage_paths diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb index d40d280140a..80c51cb5a72 100644 --- a/app/services/groups/destroy_service.rb +++ b/app/services/groups/destroy_service.rb @@ -1,8 +1,7 @@ module Groups class DestroyService < Groups::BaseService def async_execute - # Soft delete via paranoia gem - group.destroy + group.soft_delete_without_removing_associations job_id = GroupDestroyWorker.perform_async(group.id, current_user.id) Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}") end diff --git a/changelogs/unreleased/sh-fix-project-destroy-in-namespace.yml b/changelogs/unreleased/sh-fix-project-destroy-in-namespace.yml new file mode 100644 index 00000000000..9309f961345 --- /dev/null +++ b/changelogs/unreleased/sh-fix-project-destroy-in-namespace.yml @@ -0,0 +1,4 @@ +--- +title: Defer project destroys within a namespace in Groups::DestroyService#async_execute +merge_request: +author: diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index d4f898f6d9f..62c4ea01ce1 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -342,6 +342,17 @@ describe Namespace, models: true do end end + describe '#soft_delete_without_removing_associations' do + let(:project1) { create(:project_empty_repo, namespace: namespace) } + + it 'updates the deleted_at timestamp but preserves projects' do + namespace.soft_delete_without_removing_associations + + expect(Project.all).to include(project1) + expect(namespace.deleted_at).not_to be_nil + end + end + describe '#user_ids_for_project_authorizations' do it 'returns the user IDs for which to refresh authorizations' do expect(namespace.user_ids_for_project_authorizations) diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb index a37257d1bf4..d59b37bee36 100644 --- a/spec/services/groups/destroy_service_spec.rb +++ b/spec/services/groups/destroy_service_spec.rb @@ -15,6 +15,14 @@ describe Groups::DestroyService, services: true do group.add_user(user, Gitlab::Access::OWNER) end + def destroy_group(group, user, async) + if async + Groups::DestroyService.new(group, user).async_execute + else + Groups::DestroyService.new(group, user).execute + end + end + shared_examples 'group destruction' do |async| context 'database records' do before do @@ -30,30 +38,14 @@ describe Groups::DestroyService, services: true do context 'file system' do context 'Sidekiq inline' do before do - # Run sidekiq immediatly to check that renamed dir will be removed + # Run sidekiq immediately to check that renamed dir will be removed Sidekiq::Testing.inline! { destroy_group(group, user, async) } end - it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey } - it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey } - end - - context 'Sidekiq fake' do - before do - # Don't run sidekiq to check if renamed repository exists - Sidekiq::Testing.fake! { destroy_group(group, user, async) } + it 'verifies that paths have been deleted' do + expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey + expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey end - - it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey } - it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy } - end - end - - def destroy_group(group, user, async) - if async - Groups::DestroyService.new(group, user).async_execute - else - Groups::DestroyService.new(group, user).execute end end end @@ -61,6 +53,26 @@ describe Groups::DestroyService, services: true do describe 'asynchronous delete' do it_behaves_like 'group destruction', true + context 'Sidekiq fake' do + before do + # Don't run Sidekiq to verify that group and projects are not actually destroyed + Sidekiq::Testing.fake! { destroy_group(group, user, true) } + end + + after do + # Clean up stale directories + gitlab_shell.rm_namespace(project.repository_storage_path, group.path) + gitlab_shell.rm_namespace(project.repository_storage_path, remove_path) + end + + it 'verifies original paths and projects still exist' do + expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_truthy + expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey + expect(Project.unscoped.count).to eq(1) + expect(Group.unscoped.count).to eq(2) + end + end + context 'potential race conditions' do context "when the `GroupDestroyWorker` task runs immediately" do it "deletes the group" do |