Fixed legacy storage renaming code

During a previous refactor on project model, code related to the
hashed storage was extracted into AfterRenameService, see
4b9c17f196bab6075563f62d01f9db65c1a0515c.

The "path_before" was changed from using `previous_changes['path']` to
`path_was`. They are not equivalent. `path_was` exists reliably only
*before* persisting to the database. After database persistence is
confirmed, the value is moved to `previous_changes[:attribute_name]`.

Because the repository/attachments rename or storage upgrade happens
after it was persisted to the database, we were in fact not informing
the right parameters (and therefore not doing what it was supposed to).

3 files changed, 61 insertions, 50 deletions

diff --git a/app/services/projects/after_rename_service.rb b/app/services/projects/after_rename_service.rb
index aa9b253eb20..9ace1f85336 100644
--- a/app/services/projects/after_rename_service.rb
+++ b/app/services/projects/after_rename_service.rb
@@ -27,7 +27,7 @@ module Projects
       @full_path_after = project.build_full_path
 
       # The path of just the project, before the rename took place.
-      @path_before = project.path_was
+      @path_before = project.previous_changes['path'].first
     end
 
     def execute
diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb
index dd1b9680ece..d5a3a4043f3 100644
--- a/app/services/projects/update_service.rb
+++ b/app/services/projects/update_service.rb
@@ -67,7 +67,7 @@ module Projects
       end
 
       if project.previous_changes.include?('path')
-        AfterRenameService.new(project).execute
+        after_rename_service(project).execute
       else
         system_hook_service.execute_hooks_for(project, :update)
       end
@@ -75,6 +75,10 @@ module Projects
       update_pages_config if changing_pages_related_config?
     end
 
+    def after_rename_service(project)
+      AfterRenameService.new(project)
+    end
+
     def changing_pages_related_config?
       changing_pages_https_only? || changing_pages_access_level?
     end
diff --git a/spec/services/projects/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb
index 59c08b30f9f..7f272ffcd7b 100644
--- a/spec/services/projects/after_rename_service_spec.rb
+++ b/spec/services/projects/after_rename_service_spec.rb
@@ -4,26 +4,30 @@ 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!(:path_after_rename) { "#{project.path}-renamed" }
+
+  subject(:service_execute) do
+    # AfterRenameService is called by UpdateService after a successful model.update
+    # We need to simulate that here in order to populate the correct Dirty attributes to
+    # actually test the behavior of this class
+    project.update(path: path_after_rename)
+    described_class.new(project).execute
+  end
 
   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, :with_avatar, :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
 
@@ -32,12 +36,12 @@ describe Projects::AfterRenameService do
 
         expect(gitlab_shell).to receive(:mv_repository)
           .ordered
-          .with(project.repository_storage, "#{project.namespace.full_path}/foo", "#{project.full_path}")
+          .with(project.repository_storage, "#{project.namespace.full_path}/#{path_before_rename}", "#{project.namespace.full_path}/#{path_after_rename}")
           .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")
+          .with(project.repository_storage, "#{project.namespace.full_path}/#{path_before_rename}.wiki", "#{project.namespace.full_path}/#{path_after_rename}.wiki")
           .and_return(true)
 
         expect_any_instance_of(SystemHooksService)
@@ -46,11 +50,11 @@ describe Projects::AfterRenameService do
 
         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)
 
-        described_class.new(project).execute
+        service_execute
       end
 
       context 'container registry with images' do
@@ -63,8 +67,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 +79,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 +91,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,17 +104,29 @@ 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
-      let(:project) { create(:project, :repository, skip_disk_validation: true) }
+      let(:project) { create(:project, :repository, :with_avatar,  skip_disk_validation: true) }
       let(:gitlab_shell) { Gitlab::Shell.new }
       let(:hash) { Digest::SHA2.hexdigest(project.id.to_s) }
       let(:hashed_prefix) { File.join('@hashed', hash[0..1], hash[2..3]) }
@@ -123,25 +136,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 +152,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 +165,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 +174,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
+
+            service_execute
+            expect(project.reload.hashed_storage?(:attachments)).to be_truthy
 
-            described_class.new(project).execute
+            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,