Refactored AfterRenameService to reduce coupling

We still rely on the Dirty API for project rename (before/after) values,
but we don't access the dirty api from the service class anymore.

The previous value is now part of the initialization, which makes it
easier to test and the behavior is clearer.

The same was done with the `rename_repo` on the Storage classes, we now
provide before and after values as part of the method signature.

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

@@ -12,22 +12,27 @@ module Projects
   #
   #     Projects::AfterRenameService.new(project).execute
   class AfterRenameService
-    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.previous_changes['path'].first
+    # @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
@@ -61,7 +66,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

@@ -76,7 +76,10 @@ module Projects
     end
 
     def after_rename_service(project)
-      AfterRenameService.new(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?

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

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

@@ -7,19 +7,13 @@ describe Projects::AfterRenameService do
   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" }
-
-  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
+  let!(:full_path_after_rename) { "#{project.full_path}-renamed" }
 
   describe '#execute' do
     context 'using legacy storage' do
-      let(:project) { create(:project, :repository, :with_avatar, :legacy_storage) }
+      let(:project) { create(:project, :repository, :wiki_repo, :legacy_storage) }
       let(:project_storage) { project.send(:storage) }
       let(:gitlab_shell) { Gitlab::Shell.new }
 
@@ -34,16 +28,6 @@ describe Projects::AfterRenameService do
       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}/#{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}/#{path_before_rename}.wiki", "#{project.namespace.full_path}/#{path_after_rename}.wiki")
-          .and_return(true)
-
         expect_any_instance_of(SystemHooksService)
           .to receive(:execute_hooks_for)
           .with(project, :rename)
@@ -52,9 +36,13 @@ describe Projects::AfterRenameService do
           .to receive(:rename_project)
           .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")
 
         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
@@ -126,7 +114,7 @@ describe Projects::AfterRenameService do
     end
 
     context 'using hashed storage' do
-      let(:project) { create(:project, :repository, :with_avatar,  skip_disk_validation: true) }
+      let(:project) { create(:project, :repository, 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]) }
@@ -222,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