Merge branch '56636-hashed-storage-afterrenameservice' into 'master'

Hashed Storage: `AfterRenameService` was not renaming Pages or Uploads folder on legacy storage

Closes #56636

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

3 files changed, 72 insertions, 62 deletions

diff --git a/spec/migrations/rename_more_reserved_project_names_spec.rb b/spec/migrations/rename_more_reserved_project_names_spec.rb
index baf16c2ce53..80ae209e9d1 100644
--- a/spec/migrations/rename_more_reserved_project_names_spec.rb
+++ b/spec/migrations/rename_more_reserved_project_names_spec.rb
@@ -37,9 +37,8 @@ describe RenameMoreReservedProjectNames, :delete do
             .to receive(:execute)
             .and_raise(Projects::AfterRenameService::RenameFailedError)
 
-          allow(Projects::AfterRenameService)
-            .to receive(:new)
-            .with(project)
+          expect(migration)
+            .to receive(:after_rename_service)
             .and_return(service)
         end
 
diff --git a/spec/migrations/rename_reserved_project_names_spec.rb b/spec/migrations/rename_reserved_project_names_spec.rb
index 7818aa0d560..93e5c032287 100644
--- a/spec/migrations/rename_reserved_project_names_spec.rb
+++ b/spec/migrations/rename_reserved_project_names_spec.rb
@@ -41,9 +41,8 @@ describe RenameReservedProjectNames, :migration, schema: :latest do
             .to receive(:execute)
             .and_raise(Projects::AfterRenameService::RenameFailedError)
 
-          allow(Projects::AfterRenameService)
-            .to receive(:new)
-            .with(project)
+          expect(migration)
+            .to receive(:after_rename_service)
             .and_return(service)
         end
 
diff --git a/spec/services/projects/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb
index 59c08b30f9f..bc5366a3339 100644
--- a/spec/services/projects/after_rename_service_spec.rb
+++ b/spec/services/projects/after_rename_service_spec.rb
@@ -4,53 +4,45 @@ require 'spec_helper'
 
 describe Projects::AfterRenameService do
   let(:rugged_config) { rugged_repo(project.repository).config }
+  let(:legacy_storage) { Storage::LegacyProject.new(project) }
+  let(:hashed_storage) { Storage::HashedProject.new(project) }
+  let!(:path_before_rename) { project.path }
+  let!(:full_path_before_rename) { project.full_path }
+  let!(:path_after_rename) { "#{project.path}-renamed" }
+  let!(:full_path_after_rename) { "#{project.full_path}-renamed" }
 
   describe '#execute' do
     context 'using legacy storage' do
-      let(:project) { create(:project, :repository, :legacy_storage) }
-      let(:gitlab_shell) { Gitlab::Shell.new }
+      let(:project) { create(:project, :repository, :wiki_repo, :legacy_storage) }
       let(:project_storage) { project.send(:storage) }
+      let(:gitlab_shell) { Gitlab::Shell.new }
 
       before do
         # Project#gitlab_shell returns a new instance of Gitlab::Shell on every
         # call. This makes testing a bit easier.
         allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
 
-        allow(project)
-          .to receive(:previous_changes)
-          .and_return('path' => ['foo'])
-
-        allow(project)
-          .to receive(:path_was)
-          .and_return('foo')
-
         stub_feature_flags(skip_hashed_storage_upgrade: false)
       end
 
       it 'renames a repository' do
         stub_container_registry_config(enabled: false)
 
-        expect(gitlab_shell).to receive(:mv_repository)
-          .ordered
-          .with(project.repository_storage, "#{project.namespace.full_path}/foo", "#{project.full_path}")
-          .and_return(true)
-
-        expect(gitlab_shell).to receive(:mv_repository)
-          .ordered
-          .with(project.repository_storage, "#{project.namespace.full_path}/foo.wiki", "#{project.full_path}.wiki")
-          .and_return(true)
-
         expect_any_instance_of(SystemHooksService)
           .to receive(:execute_hooks_for)
           .with(project, :rename)
 
         expect_any_instance_of(Gitlab::UploadsTransfer)
           .to receive(:rename_project)
-          .with('foo', project.path, project.namespace.full_path)
+          .with(path_before_rename, path_after_rename, project.namespace.full_path)
 
-        expect(project).to receive(:expire_caches_before_rename)
+        expect_repository_exist("#{full_path_before_rename}.git")
+        expect_repository_exist("#{full_path_before_rename}.wiki.git")
 
-        described_class.new(project).execute
+        service_execute
+
+        expect_repository_exist("#{full_path_after_rename}.git")
+        expect_repository_exist("#{full_path_after_rename}.wiki.git")
       end
 
       context 'container registry with images' do
@@ -63,8 +55,7 @@ describe Projects::AfterRenameService do
         end
 
         it 'raises a RenameFailedError' do
-          expect { described_class.new(project).execute }
-            .to raise_error(described_class::RenameFailedError)
+          expect { service_execute }.to raise_error(described_class::RenameFailedError)
         end
       end
 
@@ -76,7 +67,7 @@ describe Projects::AfterRenameService do
         it 'moves pages folder to new location' do
           expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
 
-          described_class.new(project).execute
+          service_execute
         end
       end
 
@@ -88,14 +79,12 @@ describe Projects::AfterRenameService do
         it 'moves uploads folder to new location' do
           expect_any_instance_of(Gitlab::UploadsTransfer).to receive(:rename_project)
 
-          described_class.new(project).execute
+          service_execute
         end
       end
 
       it 'updates project full path in .git/config' do
-        allow(project_storage).to receive(:rename_repo).and_return(true)
-
-        described_class.new(project).execute
+        service_execute
 
         expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
       end
@@ -103,13 +92,25 @@ describe Projects::AfterRenameService do
       it 'updates storage location' do
         allow(project_storage).to receive(:rename_repo).and_return(true)
 
-        described_class.new(project).execute
+        service_execute
 
         expect(project.project_repository).to have_attributes(
           disk_path: project.disk_path,
           shard_name: project.repository_storage
         )
       end
+
+      context 'with hashed storage upgrade when renaming enabled' do
+        it 'calls HashedStorageMigrationService with correct options' do
+          stub_application_setting(hashed_storage_enabled: true)
+
+          expect_next_instance_of(::Projects::HashedStorageMigrationService) do |service|
+            expect(service).to receive(:execute).and_return(true)
+          end
+
+          service_execute
+        end
+      end
     end
 
     context 'using hashed storage' do
@@ -123,25 +124,11 @@ describe Projects::AfterRenameService do
         # Project#gitlab_shell returns a new instance of Gitlab::Shell on every
         # call. This makes testing a bit easier.
         allow(project).to receive(:gitlab_shell).and_return(gitlab_shell)
-        allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
 
         stub_feature_flags(skip_hashed_storage_upgrade: false)
         stub_application_setting(hashed_storage_enabled: true)
       end
 
-      context 'migration to hashed storage' do
-        it 'calls HashedStorageMigrationService with correct options' do
-          project = create(:project, :repository, :legacy_storage)
-          allow(project).to receive(:previous_changes).and_return('path' => ['foo'])
-
-          expect_next_instance_of(::Projects::HashedStorageMigrationService) do |service|
-            expect(service).to receive(:execute).and_return(true)
-          end
-
-          described_class.new(project).execute
-        end
-      end
-
       it 'renames a repository' do
         stub_container_registry_config(enabled: false)
 
@@ -153,7 +140,7 @@ describe Projects::AfterRenameService do
 
         expect(project).to receive(:expire_caches_before_rename)
 
-        described_class.new(project).execute
+        service_execute
       end
 
       context 'container registry with images' do
@@ -166,7 +153,7 @@ describe Projects::AfterRenameService do
         end
 
         it 'raises a RenameFailedError' do
-          expect { described_class.new(project).execute }
+          expect { service_execute }
             .to raise_error(described_class::RenameFailedError)
         end
       end
@@ -175,38 +162,46 @@ describe Projects::AfterRenameService do
         it 'moves pages folder to new location' do
           expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
 
-          described_class.new(project).execute
+          service_execute
         end
       end
 
       context 'attachments' do
+        let(:uploader) { create(:upload, :issuable_upload, :with_file, model: project) }
+        let(:file_uploader) { build(:file_uploader, project: project) }
+        let(:legacy_storage_path) { File.join(file_uploader.root, legacy_storage.disk_path) }
+        let(:hashed_storage_path) { File.join(file_uploader.root, hashed_storage.disk_path) }
+
         it 'keeps uploads folder location unchanged' do
           expect_any_instance_of(Gitlab::UploadsTransfer).not_to receive(:rename_project)
 
-          described_class.new(project).execute
+          service_execute
         end
 
         context 'when not rolled out' do
           let(:project) { create(:project, :repository, storage_version: 1, skip_disk_validation: true) }
 
-          it 'moves pages folder to hashed storage' do
-            expect_next_instance_of(Projects::HashedStorage::MigrateAttachmentsService) do |service|
-              expect(service).to receive(:execute)
-            end
+          it 'moves attachments folder to hashed storage' do
+            expect(File.directory?(legacy_storage_path)).to be_truthy
+            expect(File.directory?(hashed_storage_path)).to be_falsey
 
-            described_class.new(project).execute
+            service_execute
+            expect(project.reload.hashed_storage?(:attachments)).to be_truthy
+
+            expect(File.directory?(legacy_storage_path)).to be_falsey
+            expect(File.directory?(hashed_storage_path)).to be_truthy
           end
         end
       end
 
       it 'updates project full path in .git/config' do
-        described_class.new(project).execute
+        service_execute
 
         expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
       end
 
       it 'updates storage location' do
-        described_class.new(project).execute
+        service_execute
 
         expect(project.project_repository).to have_attributes(
           disk_path: project.disk_path,
@@ -215,4 +210,21 @@ describe Projects::AfterRenameService do
       end
     end
   end
+
+  def service_execute
+    # AfterRenameService is called by UpdateService after a successful model.update
+    # the initialization will include before and after paths values
+    project.update(path: path_after_rename)
+
+    described_class.new(project, path_before: path_before_rename, full_path_before: full_path_before_rename).execute
+  end
+
+  def expect_repository_exist(full_path_with_extension)
+    expect(
+      gitlab_shell.exists?(
+        project.repository_storage,
+        full_path_with_extension
+      )
+    ).to be_truthy
+  end
 end