Commit 30a43b7e authored by Nick Thomas's avatar Nick Thomas

Fix export removal for hashed-storage projects within a renamed or deleted namespace

parent f5990e44
...@@ -87,20 +87,10 @@ module Storage ...@@ -87,20 +87,10 @@ module Storage
remove_exports! remove_exports!
end end
def remove_exports! def remove_legacy_exports!
Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete)) legacy_export_path = File.join(Gitlab::ImportExport.storage_path, full_path_was)
end
def export_path
File.join(Gitlab::ImportExport.storage_path, full_path_was)
end
def full_path_was FileUtils.rm_rf(legacy_export_path)
if parent
parent.full_path + '/' + path_was
else
path_was
end
end end
end end
end end
...@@ -274,12 +274,6 @@ class Group < Namespace ...@@ -274,12 +274,6 @@ class Group < Namespace
list_of_ids.reverse.map { |group| variables[group.id] }.compact.flatten list_of_ids.reverse.map { |group| variables[group.id] }.compact.flatten
end end
def full_path_was
return path_was unless has_parent?
"#{parent.full_path}/#{path_was}"
end
def group_member(user) def group_member(user)
if group_members.loaded? if group_members.loaded?
group_members.find { |gm| gm.user_id == user.id } group_members.find { |gm| gm.user_id == user.id }
......
...@@ -221,6 +221,24 @@ class Namespace < ActiveRecord::Base ...@@ -221,6 +221,24 @@ class Namespace < ActiveRecord::Base
has_parent? has_parent?
end end
def full_path_was
return path_was unless has_parent?
"#{parent.full_path}/#{path_was}"
end
# Exports belonging to projects with legacy storage are placed in a common
# subdirectory of the namespace, so a simple `rm -rf` is sufficient to remove
# them.
#
# Exports of projects using hashed storage are placed in a location defined
# only by the project ID, so each must be removed individually.
def remove_exports!
remove_legacy_exports!
all_projects.with_storage_feature(:repository).find_each(&:remove_exports)
end
private private
def refresh_access_of_projects_invited_groups def refresh_access_of_projects_invited_groups
......
...@@ -69,6 +69,7 @@ class Project < ActiveRecord::Base ...@@ -69,6 +69,7 @@ class Project < ActiveRecord::Base
before_destroy :remove_private_deploy_keys before_destroy :remove_private_deploy_keys
after_destroy -> { run_after_commit { remove_pages } } after_destroy -> { run_after_commit { remove_pages } }
after_destroy :remove_exports
after_validation :check_pending_delete after_validation :check_pending_delete
...@@ -1525,6 +1526,8 @@ class Project < ActiveRecord::Base ...@@ -1525,6 +1526,8 @@ class Project < ActiveRecord::Base
end end
def export_path def export_path
return nil unless namespace.present? || hashed_storage?(:repository)
File.join(Gitlab::ImportExport.storage_path, disk_path) File.join(Gitlab::ImportExport.storage_path, disk_path)
end end
...@@ -1533,8 +1536,9 @@ class Project < ActiveRecord::Base ...@@ -1533,8 +1536,9 @@ class Project < ActiveRecord::Base
end end
def remove_exports def remove_exports
_, status = Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete)) return nil unless export_path.present?
status.zero?
FileUtils.rm_rf(export_path)
end end
def full_path_slug def full_path_slug
......
---
title: Fix export removal for hashed-storage projects within a renamed or deleted
namespace
merge_request: 16658
author:
type: fixed
...@@ -93,6 +93,12 @@ FactoryBot.define do ...@@ -93,6 +93,12 @@ FactoryBot.define do
avatar { fixture_file_upload('spec/fixtures/dk.png') } avatar { fixture_file_upload('spec/fixtures/dk.png') }
end end
trait :with_export do
after(:create) do |project, evaluator|
ProjectExportWorker.new.perform(project.creator.id, project.id)
end
end
trait :broken_storage do trait :broken_storage do
after(:create) do |project| after(:create) do |project|
project.update_column(:repository_storage, 'broken') project.update_column(:repository_storage, 'broken')
......
require 'spec_helper' require 'spec_helper'
feature 'Import/Export - Namespace export file cleanup', :js do feature 'Import/Export - Namespace export file cleanup', :js do
let(:export_path) { "#{Dir.tmpdir}/import_file_spec" } let(:export_path) { Dir.mktmpdir('namespace_export_file_spec') }
let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys }
let(:project) { create(:project) } before do
allow(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
background do
allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
end end
after do after do
FileUtils.rm_rf(export_path, secure: true) FileUtils.rm_rf(export_path, secure: true)
end end
context 'admin user' do shared_examples_for 'handling project exports on namespace change' do
let!(:old_export_path) { project.export_path }
before do before do
sign_in(create(:admin)) sign_in(create(:admin))
setup_export_project
end end
context 'moving the namespace' do context 'moving the namespace' do
scenario 'removes the export file' do it 'removes the export file' do
setup_export_project
old_export_path = project.export_path.dup
expect(File).to exist(old_export_path) expect(File).to exist(old_export_path)
project.namespace.update(path: 'new_path') project.namespace.update!(path: build(:namespace).path)
expect(File).not_to exist(old_export_path) expect(File).not_to exist(old_export_path)
end end
end end
context 'deleting the namespace' do context 'deleting the namespace' do
scenario 'removes the export file' do it 'removes the export file' do
setup_export_project
old_export_path = project.export_path.dup
expect(File).to exist(old_export_path) expect(File).to exist(old_export_path)
project.namespace.destroy project.namespace.destroy
...@@ -46,17 +39,29 @@ feature 'Import/Export - Namespace export file cleanup', :js do ...@@ -46,17 +39,29 @@ feature 'Import/Export - Namespace export file cleanup', :js do
expect(File).not_to exist(old_export_path) expect(File).not_to exist(old_export_path)
end end
end end
end
def setup_export_project describe 'legacy storage' do
visit edit_project_path(project) let(:project) { create(:project) }
expect(page).to have_content('Export project') it_behaves_like 'handling project exports on namespace change'
end
describe 'hashed storage' do
let(:project) { create(:project, :hashed) }
find(:link, 'Export project').send_keys(:return) it_behaves_like 'handling project exports on namespace change'
end
visit edit_project_path(project) def setup_export_project
visit edit_project_path(project)
expect(page).to have_content('Download export') expect(page).to have_content('Export project')
end
find(:link, 'Export project').send_keys(:return)
visit edit_project_path(project)
expect(page).to have_content('Download export')
end end
end end
...@@ -596,4 +596,24 @@ describe Namespace do ...@@ -596,4 +596,24 @@ describe Namespace do
end end
end end
end end
describe '#remove_exports' do
let(:legacy_project) { create(:project, :with_export, namespace: namespace) }
let(:hashed_project) { create(:project, :with_export, :hashed, namespace: namespace) }
let(:export_path) { Dir.mktmpdir('namespace_remove_exports_spec') }
let(:legacy_export) { legacy_project.export_project_path }
let(:hashed_export) { hashed_project.export_project_path }
it 'removes exports for legacy and hashed projects' do
allow(Gitlab::ImportExport).to receive(:storage_path) { export_path }
expect(File.exist?(legacy_export)).to be_truthy
expect(File.exist?(hashed_export)).to be_truthy
namespace.remove_exports!
expect(File.exist?(legacy_export)).to be_falsy
expect(File.exist?(hashed_export)).to be_falsy
end
end
end end
...@@ -2503,6 +2503,37 @@ describe Project do ...@@ -2503,6 +2503,37 @@ describe Project do
end end
end end
describe '#remove_exports' do
let(:project) { create(:project, :with_export) }
it 'removes the exports directory for the project' do
expect(File.exist?(project.export_path)).to be_truthy
allow(FileUtils).to receive(:rm_rf).and_call_original
expect(FileUtils).to receive(:rm_rf).with(project.export_path).and_call_original
project.remove_exports
expect(File.exist?(project.export_path)).to be_falsy
end
it 'is a no-op when there is no namespace' do
export_path = project.export_path
project.update_column(:namespace_id, nil)
expect(FileUtils).not_to receive(:rm_rf).with(export_path)
project.remove_exports
expect(File.exist?(export_path)).to be_truthy
end
it 'is run when the project is destroyed' do
expect(project).to receive(:remove_exports).and_call_original
project.destroy
end
end
describe '#forks_count' do describe '#forks_count' do
it 'returns the number of forks' do it 'returns the number of forks' do
project = build(:project) project = build(:project)
......
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