Merge branch 'mk/fix-move-upload-files-on-group-transfer' into 'master'

Fix moving local, unhashed upload or pages directories during group transfer

Closes #43993

See merge request gitlab-org/gitlab-ce!17658

5 files changed, 214 insertions, 30 deletions

diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb
index 67a988addbe..f05e606995d 100644
--- a/app/models/concerns/storage/legacy_namespace.rb
+++ b/app/models/concerns/storage/legacy_namespace.rb
@@ -7,29 +7,24 @@ module Storage
         raise Gitlab::UpdatePathError.new('Namespace cannot be moved, because at least one project has tags in container registry')
       end
 
-      expires_full_path_cache
-
-      # Move the namespace directory in all storage paths used by member projects
-      repository_storage_paths.each do |repository_storage_path|
-        # Ensure old directory exists before moving it
-        gitlab_shell.add_namespace(repository_storage_path, full_path_was)
-
-        # Ensure new directory exists before moving it (if there's a parent)
-        gitlab_shell.add_namespace(repository_storage_path, parent.full_path) if parent
+      parent_was = if parent_changed? && parent_id_was.present?
+                     Namespace.find(parent_id_was) # raise NotFound early if needed
+                   end
 
-        unless gitlab_shell.mv_namespace(repository_storage_path, full_path_was, full_path)
+      expires_full_path_cache
 
-          Rails.logger.error "Exception moving path #{repository_storage_path} from #{full_path_was} to #{full_path}"
+      move_repositories
 
-          # if we cannot move namespace directory we should rollback
-          # db changes in order to prevent out of sync between db and fs
-          raise Gitlab::UpdatePathError.new('namespace directory cannot be moved')
-        end
+      if parent_changed?
+        former_parent_full_path = parent_was&.full_path
+        parent_full_path = parent&.full_path
+        Gitlab::UploadsTransfer.new.move_namespace(path, former_parent_full_path, parent_full_path)
+        Gitlab::PagesTransfer.new.move_namespace(path, former_parent_full_path, parent_full_path)
+      else
+        Gitlab::UploadsTransfer.new.rename_namespace(full_path_was, full_path)
+        Gitlab::PagesTransfer.new.rename_namespace(full_path_was, full_path)
       end
 
-      Gitlab::UploadsTransfer.new.rename_namespace(full_path_was, full_path)
-      Gitlab::PagesTransfer.new.rename_namespace(full_path_was, full_path)
-
       remove_exports!
 
       # If repositories moved successfully we need to
@@ -57,6 +52,26 @@ module Storage
 
     private
 
+    def move_repositories
+      # Move the namespace directory in all storage paths used by member projects
+      repository_storage_paths.each do |repository_storage_path|
+        # Ensure old directory exists before moving it
+        gitlab_shell.add_namespace(repository_storage_path, full_path_was)
+
+        # Ensure new directory exists before moving it (if there's a parent)
+        gitlab_shell.add_namespace(repository_storage_path, parent.full_path) if parent
+
+        unless gitlab_shell.mv_namespace(repository_storage_path, full_path_was, full_path)
+
+          Rails.logger.error "Exception moving path #{repository_storage_path} from #{full_path_was} to #{full_path}"
+
+          # if we cannot move namespace directory we should rollback
+          # db changes in order to prevent out of sync between db and fs
+          raise Gitlab::UpdatePathError.new('namespace directory cannot be moved')
+        end
+      end
+    end
+
     def old_repository_storage_paths
       @old_repository_storage_paths ||= repository_storage_paths
     end
diff --git a/changelogs/unreleased/mk-fix-move-upload-files-on-group-transfer.yml b/changelogs/unreleased/mk-fix-move-upload-files-on-group-transfer.yml
new file mode 100644
index 00000000000..ba366b81600
--- /dev/null
+++ b/changelogs/unreleased/mk-fix-move-upload-files-on-group-transfer.yml
@@ -0,0 +1,5 @@
+---
+title: Fix missing uploads after group transfer
+merge_request: 17658
+author:
+type: fixed
diff --git a/lib/gitlab/project_transfer.rb b/lib/gitlab/project_transfer.rb
index 1bba0b78e2f..690c38737c0 100644
--- a/lib/gitlab/project_transfer.rb
+++ b/lib/gitlab/project_transfer.rb
@@ -1,13 +1,19 @@
 module Gitlab
+  # This class is used to move local, unhashed files owned by projects to their new location
   class ProjectTransfer
-    def move_project(project_path, namespace_path_was, namespace_path)
-      new_namespace_folder = File.join(root_dir, namespace_path)
-      FileUtils.mkdir_p(new_namespace_folder) unless Dir.exist?(new_namespace_folder)
-      from = File.join(root_dir, namespace_path_was, project_path)
-      to = File.join(root_dir, namespace_path, project_path)
+    # nil parent_path (or parent_path_was) represents a root namespace
+    def move_namespace(path, parent_path_was, parent_path)
+      parent_path_was ||= ''
+      parent_path ||= ''
+      new_parent_folder = File.join(root_dir, parent_path)
+      FileUtils.mkdir_p(new_parent_folder)
+      from = File.join(root_dir, parent_path_was, path)
+      to = File.join(root_dir, parent_path, path)
       move(from, to, "")
     end
 
+    alias_method :move_project, :move_namespace
+
     def rename_project(path_was, path, namespace_path)
       base_dir = File.join(root_dir, namespace_path)
       move(path_was, path, base_dir)
diff --git a/spec/lib/gitlab/project_transfer_spec.rb b/spec/lib/gitlab/project_transfer_spec.rb
index 10c5fb148cd..0b9b1f537b5 100644
--- a/spec/lib/gitlab/project_transfer_spec.rb
+++ b/spec/lib/gitlab/project_transfer_spec.rb
@@ -21,30 +21,77 @@ describe Gitlab::ProjectTransfer do
 
   describe '#move_project' do
     it "moves project upload to another namespace" do
-      FileUtils.mkdir_p(File.join(@root_dir, @namespace_path_was, @project_path))
+      path_to_be_moved = File.join(@root_dir, @namespace_path_was, @project_path)
+      expected_path = File.join(@root_dir, @namespace_path, @project_path)
+      FileUtils.mkdir_p(path_to_be_moved)
+
       @project_transfer.move_project(@project_path, @namespace_path_was, @namespace_path)
 
-      expected_path = File.join(@root_dir, @namespace_path, @project_path)
       expect(Dir.exist?(expected_path)).to be_truthy
     end
   end
 
+  describe '#move_namespace' do
+    context 'when moving namespace from root into another namespace' do
+      it "moves namespace projects' upload" do
+        child_namespace = 'test_child_namespace'
+        path_to_be_moved = File.join(@root_dir, child_namespace, @project_path)
+        expected_path = File.join(@root_dir, @namespace_path, child_namespace, @project_path)
+        FileUtils.mkdir_p(path_to_be_moved)
+
+        @project_transfer.move_namespace(child_namespace, nil, @namespace_path)
+
+        expect(Dir.exist?(expected_path)).to be_truthy
+      end
+    end
+
+    context 'when moving namespace from one parent to another' do
+      it "moves namespace projects' upload" do
+        child_namespace = 'test_child_namespace'
+        path_to_be_moved = File.join(@root_dir, @namespace_path_was, child_namespace, @project_path)
+        expected_path = File.join(@root_dir, @namespace_path, child_namespace, @project_path)
+        FileUtils.mkdir_p(path_to_be_moved)
+
+        @project_transfer.move_namespace(child_namespace, @namespace_path_was, @namespace_path)
+
+        expect(Dir.exist?(expected_path)).to be_truthy
+      end
+    end
+
+    context 'when moving namespace from having a parent to root' do
+      it "moves namespace projects' upload" do
+        child_namespace = 'test_child_namespace'
+        path_to_be_moved = File.join(@root_dir, @namespace_path_was, child_namespace, @project_path)
+        expected_path = File.join(@root_dir, child_namespace, @project_path)
+        FileUtils.mkdir_p(path_to_be_moved)
+
+        @project_transfer.move_namespace(child_namespace, @namespace_path_was, nil)
+
+        expect(Dir.exist?(expected_path)).to be_truthy
+      end
+    end
+  end
+
   describe '#rename_project' do
     it "renames project" do
-      FileUtils.mkdir_p(File.join(@root_dir, @namespace_path, @project_path_was))
+      path_to_be_moved = File.join(@root_dir, @namespace_path, @project_path_was)
+      expected_path = File.join(@root_dir, @namespace_path, @project_path)
+      FileUtils.mkdir_p(path_to_be_moved)
+
       @project_transfer.rename_project(@project_path_was, @project_path, @namespace_path)
 
-      expected_path = File.join(@root_dir, @namespace_path, @project_path)
       expect(Dir.exist?(expected_path)).to be_truthy
     end
   end
 
   describe '#rename_namespace' do
     it "renames namespace" do
-      FileUtils.mkdir_p(File.join(@root_dir, @namespace_path_was, @project_path))
+      path_to_be_moved = File.join(@root_dir, @namespace_path_was, @project_path)
+      expected_path = File.join(@root_dir, @namespace_path, @project_path)
+      FileUtils.mkdir_p(path_to_be_moved)
+
       @project_transfer.rename_namespace(@namespace_path_was, @namespace_path)
 
-      expected_path = File.join(@root_dir, @namespace_path, @project_path)
       expect(Dir.exist?(expected_path)).to be_truthy
     end
   end
diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb
index 191b60e4383..16033c9052a 100644
--- a/spec/models/namespace_spec.rb
+++ b/spec/models/namespace_spec.rb
@@ -194,11 +194,60 @@ describe Namespace do
         allow(namespace).to receive(:path).and_return('new_path')
       end
 
+<<<<<<< HEAD
       it 'raises an error about not movable project' do
         expect { namespace.move_dir }.to raise_error(/Namespace cannot be moved/)
       end
     end
+||||||| parent of 3fab8dac18... Merge branch 'mk/fix-move-upload-files-on-group-transfer' into 'master'
+      context 'with subgroups' do
+        let(:parent) { create(:group, name: 'parent', path: 'parent') }
+        let(:child) { create(:group, name: 'child', path: 'child', parent: parent) }
+        let!(:project) { create(:project_empty_repo, :legacy_storage, path: 'the-project', namespace: child, skip_disk_validation: true) }
+        let(:uploads_dir) { FileUploader.root }
+        let(:pages_dir) { File.join(TestEnv.pages_path) }
+
+        before do
+          FileUtils.mkdir_p(File.join(uploads_dir, project.full_path))
+          FileUtils.mkdir_p(File.join(pages_dir, project.full_path))
+        end
+
+        context 'renaming child' do
+          it 'correctly moves the repository, uploads and pages' do
+            expected_repository_path = File.join(TestEnv.repos_path, 'parent', 'renamed', 'the-project.git')
+            expected_upload_path = File.join(uploads_dir, 'parent', 'renamed', 'the-project')
+            expected_pages_path = File.join(pages_dir, 'parent', 'renamed', 'the-project')
+=======
+      context 'with subgroups', :nested_groups do
+        let(:parent) { create(:group, name: 'parent', path: 'parent') }
+        let(:new_parent) { create(:group, name: 'new_parent', path: 'new_parent') }
+        let(:child) { create(:group, name: 'child', path: 'child', parent: parent) }
+        let!(:project) { create(:project_empty_repo, :legacy_storage, path: 'the-project', namespace: child, skip_disk_validation: true) }
+        let(:uploads_dir) { FileUploader.root }
+        let(:pages_dir) { File.join(TestEnv.pages_path) }
+
+        def expect_project_directories_at(namespace_path)
+          expected_repository_path = File.join(TestEnv.repos_path, namespace_path, 'the-project.git')
+          expected_upload_path = File.join(uploads_dir, namespace_path, 'the-project')
+          expected_pages_path = File.join(pages_dir, namespace_path, 'the-project')
+
+          expect(File.directory?(expected_repository_path)).to be_truthy
+          expect(File.directory?(expected_upload_path)).to be_truthy
+          expect(File.directory?(expected_pages_path)).to be_truthy
+        end
+
+        before do
+          FileUtils.mkdir_p(File.join(TestEnv.repos_path, "#{project.full_path}.git"))
+          FileUtils.mkdir_p(File.join(uploads_dir, project.full_path))
+          FileUtils.mkdir_p(File.join(pages_dir, project.full_path))
+        end
+
+        context 'renaming child' do
+          it 'correctly moves the repository, uploads and pages' do
+            child.update!(path: 'renamed')
+>>>>>>> 3fab8dac18... Merge branch 'mk/fix-move-upload-files-on-group-transfer' into 'master'
 
+<<<<<<< HEAD
     context 'with subgroups' do
       let(:parent) { create(:group, name: 'parent', path: 'parent') }
       let(:child) { create(:group, name: 'child', path: 'child', parent: parent) }
@@ -210,18 +259,81 @@ describe Namespace do
         FileUtils.mkdir_p(File.join(uploads_dir, 'parent', 'child', 'the-project'))
         FileUtils.mkdir_p(File.join(pages_dir, 'parent', 'child', 'the-project'))
       end
+||||||| parent of 3fab8dac18... Merge branch 'mk/fix-move-upload-files-on-group-transfer' into 'master'
+            child.update_attributes!(path: 'renamed')
 
+            expect(File.directory?(expected_repository_path)).to be(true)
+            expect(File.directory?(expected_upload_path)).to be(true)
+            expect(File.directory?(expected_pages_path)).to be(true)
+          end
+        end
+=======
+            expect_project_directories_at('parent/renamed')
+          end
+        end
+>>>>>>> 3fab8dac18... Merge branch 'mk/fix-move-upload-files-on-group-transfer' into 'master'
+
+<<<<<<< HEAD
       context 'renaming child' do
         it 'correctly moves the repository, uploads and pages' do
           expected_repository_path = File.join(TestEnv.repos_path, 'parent', 'renamed', 'the-project.git')
           expected_upload_path = File.join(uploads_dir, 'parent', 'renamed', 'the-project')
           expected_pages_path = File.join(pages_dir, 'parent', 'renamed', 'the-project')
+||||||| parent of 3fab8dac18... Merge branch 'mk/fix-move-upload-files-on-group-transfer' into 'master'
+        context 'renaming parent' do
+          it 'correctly moves the repository, uploads and pages' do
+            expected_repository_path = File.join(TestEnv.repos_path, 'renamed', 'child', 'the-project.git')
+            expected_upload_path = File.join(uploads_dir, 'renamed', 'child', 'the-project')
+            expected_pages_path = File.join(pages_dir, 'renamed', 'child', 'the-project')
+=======
+        context 'renaming parent' do
+          it 'correctly moves the repository, uploads and pages' do
+            parent.update!(path: 'renamed')
+
+            expect_project_directories_at('renamed/child')
+          end
+        end
+
+        context 'moving from one parent to another' do
+          it 'correctly moves the repository, uploads and pages' do
+            child.update!(parent: new_parent)
+>>>>>>> 3fab8dac18... Merge branch 'mk/fix-move-upload-files-on-group-transfer' into 'master'
 
+<<<<<<< HEAD
           child.update_attributes!(path: 'renamed')
+||||||| parent of 3fab8dac18... Merge branch 'mk/fix-move-upload-files-on-group-transfer' into 'master'
+            parent.update_attributes!(path: 'renamed')
+=======
+            expect_project_directories_at('new_parent/child')
+          end
+        end
+
+        context 'moving from having a parent to root' do
+          it 'correctly moves the repository, uploads and pages' do
+            child.update!(parent: nil)
 
+            expect_project_directories_at('child')
+          end
+        end
+
+        context 'moving from root to having a parent' do
+          it 'correctly moves the repository, uploads and pages' do
+            parent.update!(parent: new_parent)
+>>>>>>> 3fab8dac18... Merge branch 'mk/fix-move-upload-files-on-group-transfer' into 'master'
+
+<<<<<<< HEAD
           expect(File.directory?(expected_repository_path)).to be(true)
           expect(File.directory?(expected_upload_path)).to be(true)
           expect(File.directory?(expected_pages_path)).to be(true)
+||||||| parent of 3fab8dac18... Merge branch 'mk/fix-move-upload-files-on-group-transfer' into 'master'
+            expect(File.directory?(expected_repository_path)).to be(true)
+            expect(File.directory?(expected_upload_path)).to be(true)
+            expect(File.directory?(expected_pages_path)).to be(true)
+          end
+=======
+            expect_project_directories_at('new_parent/parent/child')
+          end
+>>>>>>> 3fab8dac18... Merge branch 'mk/fix-move-upload-files-on-group-transfer' into 'master'
         end
       end
 
@@ -481,7 +593,6 @@ describe Namespace do
       end
     end
 
-    # Note: Group transfers are not yet implemented
     context 'when a group is transferred into a root group' do
       context 'when the root group "Share with group lock" is enabled' do
         let(:root_group) { create(:group, share_with_group_lock: true) }