Commit 1e66ff6d authored by Rémy Coutable's avatar Rémy Coutable 🔴

Merge branch '42270-fix-namespace-remove-exports-for-hashed-storage' into 'master'

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

Closes #42270

See merge request gitlab-org/gitlab-ce!16658
parents 4ff15fcf 30a43b7e
Pipeline #17093677 passed with stages
in 33 minutes and 15 seconds
......@@ -87,20 +87,10 @@ module Storage
remove_exports!
end
def remove_exports!
Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete))
end
def export_path
File.join(Gitlab::ImportExport.storage_path, full_path_was)
end
def remove_legacy_exports!
legacy_export_path = File.join(Gitlab::ImportExport.storage_path, full_path_was)
def full_path_was
if parent
parent.full_path + '/' + path_was
else
path_was
end
FileUtils.rm_rf(legacy_export_path)
end
end
end
......@@ -274,12 +274,6 @@ class Group < Namespace
list_of_ids.reverse.map { |group| variables[group.id] }.compact.flatten
end
def full_path_was
return path_was unless has_parent?
"#{parent.full_path}/#{path_was}"
end
def group_member(user)
if group_members.loaded?
group_members.find { |gm| gm.user_id == user.id }
......
......@@ -221,6 +221,24 @@ class Namespace < ActiveRecord::Base
has_parent?
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
def refresh_access_of_projects_invited_groups
......
......@@ -69,6 +69,7 @@ class Project < ActiveRecord::Base
before_destroy :remove_private_deploy_keys
after_destroy -> { run_after_commit { remove_pages } }
after_destroy :remove_exports
after_validation :check_pending_delete
......@@ -1525,6 +1526,8 @@ class Project < ActiveRecord::Base
end
def export_path
return nil unless namespace.present? || hashed_storage?(:repository)
File.join(Gitlab::ImportExport.storage_path, disk_path)
end
......@@ -1533,8 +1536,9 @@ class Project < ActiveRecord::Base
end
def remove_exports
_, status = Gitlab::Popen.popen(%W(find #{export_path} -not -path #{export_path} -delete))
status.zero?
return nil unless export_path.present?
FileUtils.rm_rf(export_path)
end
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
avatar { fixture_file_upload('spec/fixtures/dk.png') }
end
trait :with_export do
after(:create) do |project, evaluator|
ProjectExportWorker.new.perform(project.creator.id, project.id)
end
end
trait :broken_storage do
after(:create) do |project|
project.update_column(:repository_storage, 'broken')
......
require 'spec_helper'
feature 'Import/Export - Namespace export file cleanup', :js do
let(:export_path) { "#{Dir.tmpdir}/import_file_spec" }
let(:config_hash) { YAML.load_file(Gitlab::ImportExport.config_file).deep_stringify_keys }
let(:export_path) { Dir.mktmpdir('namespace_export_file_spec') }
let(:project) { create(:project) }
background do
allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
before do
allow(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
end
after do
FileUtils.rm_rf(export_path, secure: true)
end
context 'admin user' do
shared_examples_for 'handling project exports on namespace change' do
let!(:old_export_path) { project.export_path }
before do
sign_in(create(:admin))
setup_export_project
end
context 'moving the namespace' do
scenario 'removes the export file' do
setup_export_project
old_export_path = project.export_path.dup
it 'removes the export file' do
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)
end
end
context 'deleting the namespace' do
scenario 'removes the export file' do
setup_export_project
old_export_path = project.export_path.dup
it 'removes the export file' do
expect(File).to exist(old_export_path)
project.namespace.destroy
......@@ -46,17 +39,29 @@ feature 'Import/Export - Namespace export file cleanup', :js do
expect(File).not_to exist(old_export_path)
end
end
end
def setup_export_project
visit edit_project_path(project)
describe 'legacy storage' do
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')
end
expect(page).to have_content('Export project')
find(:link, 'Export project').send_keys(:return)
visit edit_project_path(project)
expect(page).to have_content('Download export')
end
end
......@@ -596,4 +596,24 @@ describe Namespace do
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
......@@ -2503,6 +2503,37 @@ describe Project do
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
it 'returns the number of forks' do
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