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

[EE backport] Hashed Storage: `AfterRenameService` was not renaming Pages or Uploads folder on legacy storage

See merge request gitlab-org/gitlab-ee!9256

app/models/storage/hashed_project.rb

@@ -31,7 +31,7 @@ module Storage
       gitlab_shell.add_namespace(repository_storage, base_dir)
     end
 
-    def rename_repo
+    def rename_repo(old_full_path: nil, new_full_path: nil)
       true
     end
 

app/models/storage/legacy_project.rb

@@ -29,18 +29,19 @@ module Storage
       gitlab_shell.add_namespace(repository_storage, base_dir)
     end
 
-    def rename_repo
-      new_full_path = project.build_full_path
+    def rename_repo(old_full_path: nil, new_full_path: nil)
+      old_full_path ||= project.full_path_was
+      new_full_path ||= project.build_full_path
 
-      if gitlab_shell.mv_repository(repository_storage, project.full_path_was, new_full_path)
+      if gitlab_shell.mv_repository(repository_storage, old_full_path, new_full_path)
         # If repository moved successfully we need to send update instructions to users.
         # However we cannot allow rollback since we moved repository
         # So we basically we mute exceptions in next actions
         begin
-          gitlab_shell.mv_repository(repository_storage, "#{project.full_path_was}.wiki", "#{new_full_path}.wiki")
+          gitlab_shell.mv_repository(repository_storage, "#{old_full_path}.wiki", "#{new_full_path}.wiki")
           return true
         rescue => e
-          Rails.logger.error "Exception renaming #{project.full_path_was} -> #{new_full_path}: #{e}"
+          Rails.logger.error "Exception renaming #{old_full_path} -> #{new_full_path}: #{e}"
           # Returning false does not rollback after_* transaction but gives
           # us information about failing some of tasks
           return false

app/services/projects/after_rename_service.rb

@@ -14,22 +14,27 @@ module Projects
   class AfterRenameService
     prepend ::EE::Projects::AfterRenameService # rubocop: disable Cop/InjectEnterpriseEditionModule
 
-    attr_reader :project, :full_path_before, :full_path_after, :path_before
+    # @return [String] The Project being renamed.
+    attr_reader :project
 
-    RenameFailedError = Class.new(StandardError)
+    # @return [String] The path slug the project was using, before the rename took place.
+    attr_reader :path_before
 
-    # @param [Project] project The Project of the repository to rename.
-    def initialize(project)
-      @project = project
+    # @return [String] The full path of the namespace + project, before the rename took place.
+    attr_reader :full_path_before
 
-      # The full path of the namespace + project, before the rename took place.
-      @full_path_before = project.full_path_was
+    # @return [String] The full path of the namespace + project, after the rename took place.
+    attr_reader :full_path_after
 
-      # The full path of the namespace + project, after the rename took place.
-      @full_path_after = project.build_full_path
+    RenameFailedError = Class.new(StandardError)
 
-      # The path of just the project, before the rename took place.
-      @path_before = project.path_was
+    # @param [Project] project The Project being renamed.
+    # @param [String] path_before The path slug the project was using, before the rename took place.
+    def initialize(project, path_before:, full_path_before:)
+      @project = project
+      @path_before = path_before
+      @full_path_before = full_path_before
+      @full_path_after = project.full_path
     end
 
     def execute
@@ -63,7 +68,7 @@ module Projects
             .new(project, full_path_before)
             .execute
         else
-          project.storage.rename_repo
+          project.storage.rename_repo(old_full_path: full_path_before, new_full_path: full_path_after)
         end
 
       rename_failed! unless success

app/services/projects/update_service.rb

@@ -69,7 +69,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
@@ -77,6 +77,13 @@ module Projects
       update_pages_config if changing_pages_related_config?
     end
 
+    def after_rename_service(project)
+      # The path slug the project was using, before the rename took place.
+      path_before = project.previous_changes['path'].first
+
+      AfterRenameService.new(project, path_before: path_before, full_path_before: project.full_path_was)
+    end
+
     def changing_pages_related_config?
       changing_pages_https_only? || changing_pages_access_level?
     end

changelogs/unreleased/56636-hashed-storage-afterrenameservice.yml

+---
+title: 'Hashed Storage: `AfterRenameService` was receiving the wrong `old_path` under some circunstances'
+merge_request: 24526
+author:
+type: fixed

db/post_migrate/20161221153951_rename_reserved_project_names.rb

@@ -113,7 +113,7 @@ class RenameReservedProjectNames < ActiveRecord::Migration[4.2]
         # Because project path update is quite complex operation we can't safely
         # copy-paste all code from GitLab. As exception we use Rails code here
         if rename_project_row(project, path)
-          Projects::AfterRenameService.new(project).execute
+          after_rename_service(project, path_was, namespace_path).execute
         end
       rescue Exception => e # rubocop: disable Lint/RescueException
         Rails.logger.error "Exception when renaming project #{id}: #{e.message}"
@@ -126,4 +126,12 @@ class RenameReservedProjectNames < ActiveRecord::Migration[4.2]
       project.update(path: path) &&
       defined?(Projects::AfterRenameService)
   end
+
+  def after_rename_service(project, path_was, namespace_path)
+    AfterRenameService.new(
+      project,
+      path_before: path_was,
+      full_path_before: "#{namespace_path}/#{path_was}"
+    ).execute
+  end
 end

db/post_migrate/20170313133418_rename_more_reserved_project_names.rb

@@ -55,7 +55,7 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration[4.2]
         # Because project path update is quite complex operation we can't safely
         # copy-paste all code from GitLab. As exception we use Rails code here
         if rename_project_row(project, path)
-          Projects::AfterRenameService.new(project).execute
+          after_rename_service(project, path_was, namespace_path).execute
         end
       rescue Exception => e # rubocop: disable Lint/RescueException
         Rails.logger.error "Exception when renaming project #{id}: #{e.message}"
@@ -68,4 +68,12 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration[4.2]
       project.update(path: path) &&
       defined?(Projects::AfterRenameService)
   end
+
+  def after_rename_service(project, path_was, namespace_path)
+    AfterRenameService.new(
+      project,
+      path_before: path_was,
+      full_path_before: "#{namespace_path}/#{path_was}"
+    ).execute
+  end
 end

ee/spec/services/projects/after_rename_service_spec.rb

@@ -8,60 +8,35 @@ describe Projects::AfterRenameService do
       set(:primary) { create(:geo_node, :primary) }
       set(:secondary) { create(:geo_node) }
       let(:project) { create(:project, :repository, :legacy_storage) }
-      let(:gitlab_shell) { Gitlab::Shell.new }
+      let!(:path_before_rename) { project.path }
+      let!(:full_path_before_rename) { project.full_path }
+      let(:path_after_rename) { "#{project.path}-renamed" }
 
       it 'logs the Geo::RepositoryRenamedEvent for project backed by hashed storage' do
-        project_hashed_storage = create(:project)
-
-        allow(project_hashed_storage)
-          .to receive(:gitlab_shell)
-          .and_return(gitlab_shell)
-
-        allow(project_hashed_storage)
-          .to receive(:previous_changes)
-          .and_return('path' => ['foo'])
-
-        allow(project_hashed_storage)
-          .to receive(:path_was)
-          .and_return('foo')
-
-        allow(gitlab_shell)
-          .to receive(:mv_repository)
-          .twice.and_return(true)
-
-        expect { described_class.new(project_hashed_storage).execute }
-          .to change(Geo::RepositoryRenamedEvent, :count)
+        expect { service_execute }.to change(Geo::RepositoryRenamedEvent, :count)
       end
 
       it 'logs the Geo::RepositoryRenamedEvent for project backed by legacy storage' do
-        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')
-
-        allow(gitlab_shell)
-          .to receive(:mv_repository)
-          .twice.and_return(true)
-
         expect(Geo::RepositoryRenamedEventStore)
           .to receive(:new)
           .with(
             project,
-            old_path: 'foo',
-            old_path_with_namespace: "#{project.namespace.full_path}/foo"
+            old_path: path_before_rename,
+            old_path_with_namespace: full_path_before_rename
           )
           .and_call_original
 
-        expect { described_class.new(project).execute }
+        expect { service_execute }
           .to change(Geo::RepositoryRenamedEvent, :count).by(1)
       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
 end

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
 

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
 

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