From fbb5953ec36ae3e53f9a07f5a0d1cf1b5e1ef209 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Wed, 10 Oct 2018 12:15:56 -0700
Subject: [PATCH] Fix project deletion when there is a export available
Project deletions were failing with "Can't modify frozen hash" because:
1. Project#remove_exports was called in the after_destroy hook
2. This would remove the file and update ImportExportUpload
3. ImportExportUpload#save would attempt to write to a destroyed model
To avoid this, we just check if ImportExportUpload has been destroyed
before attempting to save it.
This would have a side effect of not running after_commit hooks to delete the
repository on disk, making it impossible to delete the project entirely.
Closes #52362
---
app/models/project.rb | 2 +-
.../sh-fix-project-deletion-with-export.yml | 5 ++++
.../services/projects/destroy_service_spec.rb | 23 ++++++++++++++++---
3 files changed, 26 insertions(+), 4 deletions(-)
create mode 100644 changelogs/unreleased/sh-fix-project-deletion-with-export.yml
diff --git a/app/models/project.rb b/app/models/project.rb
index 05e14c578b5c3..c7ca322853f38 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1789,7 +1789,7 @@ def remove_exports
return unless export_file_exists?
import_export_upload.remove_export_file!
- import_export_upload.save
+ import_export_upload.save unless import_export_upload.destroyed?
end
def export_file_exists?
diff --git a/changelogs/unreleased/sh-fix-project-deletion-with-export.yml b/changelogs/unreleased/sh-fix-project-deletion-with-export.yml
new file mode 100644
index 0000000000000..b9437f8ad6ad1
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-project-deletion-with-export.yml
@@ -0,0 +1,5 @@
+---
+title: Fix project deletion when there is a export available
+merge_request: 22276
+author:
+type: fixed
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index beff499f2be22..1d31d26f418bb 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -65,10 +65,12 @@
context 'Sidekiq inline' do
before do
- # Run sidekiq immediatly to check that renamed repository will be removed
+ # Run sidekiq immediately to check that renamed repository will be removed
perform_enqueued_jobs { destroy_project(project, user, {}) }
end
+ it_behaves_like 'deleting the project'
+
context 'when has remote mirrors' do
let!(:project) do
create(:project, :repository, namespace: user.namespace).tap do |project|
@@ -82,13 +84,28 @@
end
end
- it_behaves_like 'deleting the project'
-
it 'invalidates personal_project_count cache' do
expect(user).to receive(:invalidate_personal_projects_count)
destroy_project(project, user)
end
+
+ context 'when project has exports' do
+ let!(:project_with_export) do
+ create(:project, :repository, namespace: user.namespace).tap do |project|
+ create(:import_export_upload,
+ project: project,
+ export_file: fixture_file_upload('spec/fixtures/project_export.tar.gz'))
+ end
+ end
+ let!(:async) { true }
+
+ it 'destroys project and export' do
+ expect { destroy_project(project_with_export, user) }.to change(ImportExportUpload, :count).by(-1)
+
+ expect(Project.all).not_to include(project_with_export)
+ end
+ end
end
context 'Sidekiq fake' do
--
GitLab