Merge branch 'refactor-project-rename-repo' into 'master'

Move Project#rename_repo to a service class

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

app/models/project.rb

@@ -1630,34 +1630,6 @@ class Project < ActiveRecord::Base
   end
   # rubocop: enable CodeReuse/ServiceClass
 
-  def rename_repo
-    path_before      = previous_changes['path'].first
-    full_path_before = full_path_was
-    full_path_after  = build_full_path
-
-    Gitlab::AppLogger.info("Attempting to rename #{full_path_was} -> #{full_path_after}")
-
-    if has_container_registry_tags?
-      Gitlab::AppLogger.info("Project #{full_path_was} cannot be renamed because container registry tags are present!")
-
-      # we currently don't support renaming repository if it contains images in container registry
-      raise StandardError.new('Project cannot be renamed, because images are present in its container registry')
-    end
-
-    expire_caches_before_rename(full_path_before)
-
-    if rename_or_migrate_repository!
-      Gitlab::AppLogger.info("Project was renamed: #{full_path_before} -> #{full_path_after}")
-      after_rename_repository(full_path_before, path_before)
-    else
-      Gitlab::AppLogger.info("Repository could not be renamed: #{full_path_before} -> #{full_path_after}")
-
-      # if we cannot move namespace directory we should rollback
-      # db changes in order to prevent out of sync between db and fs
-      raise StandardError.new('Repository cannot be renamed')
-    end
-  end
-
   def write_repository_config(gl_full_path: full_path)
     # We'd need to keep track of project full path otherwise directory tree
     # created with hashed storage enabled cannot be usefully imported using
@@ -2086,51 +2058,6 @@ class Project < ActiveRecord::Base
     auto_cancel_pending_pipelines == 'enabled'
   end
 
-  private
-
-  # rubocop: disable CodeReuse/ServiceClass
-  def rename_or_migrate_repository!
-    if Gitlab::CurrentSettings.hashed_storage_enabled? &&
-        storage_upgradable? &&
-        Feature.disabled?(:skip_hashed_storage_upgrade) # kill switch in case we need to disable upgrade behavior
-      ::Projects::HashedStorageMigrationService.new(self, full_path_was).execute
-    else
-      storage.rename_repo
-    end
-  end
-  # rubocop: enable CodeReuse/ServiceClass
-
-  def storage_upgradable?
-    storage_version != LATEST_STORAGE_VERSION
-  end
-
-  def after_rename_repository(full_path_before, path_before)
-    execute_rename_repository_hooks!(full_path_before)
-
-    write_repository_config
-
-    # We need to check if project had been rolled out to move resource to hashed storage or not and decide
-    # if we need execute any take action or no-op.
-    unless hashed_storage?(:attachments)
-      Gitlab::UploadsTransfer.new.rename_project(path_before, self.path, namespace.full_path)
-    end
-
-    Gitlab::PagesTransfer.new.rename_project(path_before, self.path, namespace.full_path)
-  end
-
-  # rubocop: disable CodeReuse/ServiceClass
-  def execute_rename_repository_hooks!(full_path_before)
-    # When we import a project overwriting the original project, there
-    # is a move operation. In that case we don't want to send the instructions.
-    send_move_instructions(full_path_before) unless import_started?
-
-    self.old_path_with_namespace = full_path_before
-    SystemHooksService.new.execute_hooks_for(self, :rename)
-
-    reload_repository!
-  end
-  # rubocop: enable CodeReuse/ServiceClass
-
   def storage
     @storage ||=
       if hashed_storage?(:repository)
@@ -2140,6 +2067,12 @@ class Project < ActiveRecord::Base
       end
   end
 
+  def storage_upgradable?
+    storage_version != LATEST_STORAGE_VERSION
+  end
+
+  private
+
   def use_hashed_storage
     if self.new_record? && Gitlab::CurrentSettings.hashed_storage_enabled
       self.storage_version = LATEST_STORAGE_VERSION

app/services/projects/after_rename_service.rb

+# frozen_string_literal: true
+
+module Projects
+  # Service class for performing operations that should take place after a
+  # project has been renamed.
+  #
+  # Example usage:
+  #
+  #     project = Project.find(42)
+  #
+  #     project.update(...)
+  #
+  #     Projects::AfterRenameService.new(project).execute
+  class AfterRenameService
+    attr_reader :project, :full_path_before, :full_path_after, :path_before
+
+    RenameFailedError = Class.new(StandardError)
+
+    # @param [Project] project The Project of the repository to rename.
+    def initialize(project)
+      @project = project
+
+      # The full path of the namespace + project, before the rename took place.
+      @full_path_before = project.full_path_was
+
+      # The full path of the namespace + project, after the rename took place.
+      @full_path_after = project.build_full_path
+
+      # The path of just the project, before the rename took place.
+      @path_before = project.path_was
+    end
+
+    def execute
+      first_ensure_no_registry_tags_are_present
+      expire_caches_before_rename
+      rename_or_migrate_repository!
+      send_move_instructions
+      execute_system_hooks
+      update_repository_configuration
+      rename_transferred_documents
+      log_completion
+    end
+
+    def first_ensure_no_registry_tags_are_present
+      return unless project.has_container_registry_tags?
+
+      raise RenameFailedError.new(
+        "Project #{full_path_before} cannot be renamed because images are " \
+          "present in its container registry"
+      )
+    end
+
+    def expire_caches_before_rename
+      project.expire_caches_before_rename(full_path_before)
+    end
+
+    def rename_or_migrate_repository!
+      success =
+        if migrate_to_hashed_storage?
+          ::Projects::HashedStorageMigrationService
+            .new(project, full_path_before)
+            .execute
+        else
+          project.storage.rename_repo
+        end
+
+      rename_failed! unless success
+    end
+
+    def send_move_instructions
+      return unless send_move_instructions?
+
+      project.send_move_instructions(full_path_before)
+    end
+
+    def execute_system_hooks
+      project.old_path_with_namespace = full_path_before
+      SystemHooksService.new.execute_hooks_for(project, :rename)
+    end
+
+    def update_repository_configuration
+      project.reload_repository!
+      project.write_repository_config
+    end
+
+    def rename_transferred_documents
+      if rename_uploads?
+        Gitlab::UploadsTransfer
+          .new
+          .rename_project(path_before, project_path, namespace_full_path)
+      end
+
+      Gitlab::PagesTransfer
+        .new
+        .rename_project(path_before, project_path, namespace_full_path)
+    end
+
+    def log_completion
+      Gitlab::AppLogger.info(
+        "Project #{project.id} has been renamed from " \
+          "#{full_path_before} to #{full_path_after}"
+      )
+    end
+
+    def migrate_to_hashed_storage?
+      Gitlab::CurrentSettings.hashed_storage_enabled? &&
+        project.storage_upgradable? &&
+        Feature.disabled?(:skip_hashed_storage_upgrade)
+    end
+
+    def send_move_instructions?
+      !project.import_started?
+    end
+
+    def rename_uploads?
+      !project.hashed_storage?(:attachments)
+    end
+
+    def project_path
+      project.path
+    end
+
+    def namespace_full_path
+      project.namespace.full_path
+    end
+
+    def rename_failed!
+      error = "Repository #{full_path_before} could not be renamed to #{full_path_after}"
+
+      Gitlab::AppLogger.error(error)
+
+      raise RenameFailedError.new(error)
+    end
+  end
+end

app/services/projects/update_service.rb

@@ -67,7 +67,7 @@ module Projects
       end
 
       if project.previous_changes.include?('path')
-        project.rename_repo
+        AfterRenameService.new(project).execute
       else
         system_hook_service.execute_hooks_for(project, :update)
       end

db/post_migrate/20161221153951_rename_reserved_project_names.rb

@@ -113,7 +113,9 @@ class RenameReservedProjectNames < ActiveRecord::Migration
       begin
         # 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
-        project.rename_repo if rename_project_row(project, path)
+        if rename_project_row(project, path)
+          Projects::AfterRenameService.new(project).execute
+        end
       rescue Exception => e # rubocop: disable Lint/RescueException
         Rails.logger.error "Exception when renaming project #{id}: #{e.message}"
       end
@@ -123,6 +125,6 @@ class RenameReservedProjectNames < ActiveRecord::Migration
   def rename_project_row(project, path)
     project.respond_to?(:update_attributes) &&
       project.update(path: path) &&
-      project.respond_to?(:rename_repo)
+      defined?(Projects::AfterRenameService)
   end
 end

db/post_migrate/20170313133418_rename_more_reserved_project_names.rb

@@ -55,7 +55,9 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration
       begin
         # 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
-        project.rename_repo if rename_project_row(project, path)
+        if rename_project_row(project, path)
+          Projects::AfterRenameService.new(project).execute
+        end
       rescue Exception => e # rubocop: disable Lint/RescueException
         Rails.logger.error "Exception when renaming project #{id}: #{e.message}"
       end
@@ -65,6 +67,6 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration
   def rename_project_row(project, path)
     project.respond_to?(:update_attributes) &&
       project.update(path: path) &&
-      project.respond_to?(:rename_repo)
+      defined?(Projects::AfterRenameService)
   end
 end

spec/migrations/rename_more_reserved_project_names_spec.rb

@@ -31,7 +31,16 @@ describe RenameMoreReservedProjectNames, :delete do
 
       context 'when exception is raised during rename' do
         before do
-          allow(project).to receive(:rename_repo).and_raise(StandardError)
+          service = instance_double('service')
+
+          allow(service)
+            .to receive(:execute)
+            .and_raise(Projects::AfterRenameService::RenameFailedError)
+
+          allow(Projects::AfterRenameService)
+            .to receive(:new)
+            .with(project)
+            .and_return(service)
         end
 
         it 'captures exception from project rename' do

spec/migrations/rename_reserved_project_names_spec.rb

@@ -35,7 +35,16 @@ describe RenameReservedProjectNames, :migration, schema: :latest do
 
       context 'when exception is raised during rename' do
         before do
-          allow(project).to receive(:rename_repo).and_raise(StandardError)
+          service = instance_double('service')
+
+          allow(service)
+            .to receive(:execute)
+            .and_raise(Projects::AfterRenameService::RenameFailedError)
+
+          allow(Projects::AfterRenameService)
+            .to receive(:new)
+            .with(project)
+            .and_return(service)
         end
 
         it 'captures exception from project rename' do

spec/models/project_spec.rb

@@ -2965,88 +2965,6 @@ describe Project do
       end
     end
 
-    describe '#rename_repo' do
-      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'])
-        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)
-
-        expect(project).to receive(:expire_caches_before_rename)
-
-        project.rename_repo
-      end
-
-      context 'container registry with images' do
-        let(:container_repository) { create(:container_repository) }
-
-        before do
-          stub_container_registry_config(enabled: true)
-          stub_container_registry_tags(repository: :any, tags: ['tag'])
-          project.container_repositories << container_repository
-        end
-
-        subject { project.rename_repo }
-
-        it { expect { subject }.to raise_error(StandardError) }
-      end
-
-      context 'gitlab pages' do
-        before do
-          expect(project_storage).to receive(:rename_repo) { true }
-        end
-
-        it 'moves pages folder to new location' do
-          expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
-
-          project.rename_repo
-        end
-      end
-
-      context 'attachments' do
-        before do
-          expect(project_storage).to receive(:rename_repo) { true }
-        end
-
-        it 'moves uploads folder to new location' do
-          expect_any_instance_of(Gitlab::UploadsTransfer).to receive(:rename_project)
-
-          project.rename_repo
-        end
-      end
-
-      it 'updates project full path in .git/config' do
-        allow(project_storage).to receive(:rename_repo).and_return(true)
-
-        project.rename_repo
-
-        expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
-      end
-    end
-
     describe '#pages_path' do
       it 'returns a path where pages are stored' do
         expect(project.pages_path).to eq(File.join(Settings.pages.path, project.namespace.full_path, project.path))
@@ -3137,91 +3055,6 @@ describe Project do
       end
     end
 
-    describe '#rename_repo' do
-      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'])
-        stub_feature_flags(skip_hashed_storage_upgrade: false)
-      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
-
-          project.rename_repo
-        end
-      end
-
-      it 'renames a repository' do
-        stub_container_registry_config(enabled: false)
-
-        expect(gitlab_shell).not_to receive(:mv_repository)
-
-        expect_any_instance_of(SystemHooksService)
-          .to receive(:execute_hooks_for)
-            .with(project, :rename)
-
-        expect(project).to receive(:expire_caches_before_rename)
-
-        project.rename_repo
-      end
-
-      context 'container registry with images' do
-        let(:container_repository) { create(:container_repository) }
-
-        before do
-          stub_container_registry_config(enabled: true)
-          stub_container_registry_tags(repository: :any, tags: ['tag'])
-          project.container_repositories << container_repository
-        end
-
-        subject { project.rename_repo }
-
-        it { expect { subject }.to raise_error(StandardError) }
-      end
-
-      context 'gitlab pages' do
-        it 'moves pages folder to new location' do
-          expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
-
-          project.rename_repo
-        end
-      end
-
-      context 'attachments' do
-        it 'keeps uploads folder location unchanged' do
-          expect_any_instance_of(Gitlab::UploadsTransfer).not_to receive(:rename_project)
-
-          project.rename_repo
-        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
-
-            project.rename_repo
-          end
-        end
-      end
-
-      it 'updates project full path in .git/config' do
-        project.rename_repo
-
-        expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
-      end
-    end
-
     describe '#pages_path' do
       it 'returns a path where pages are stored' do
         expect(project.pages_path).to eq(File.join(Settings.pages.path, project.namespace.full_path, project.path))

spec/services/projects/after_rename_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::AfterRenameService do
+  let(:rugged_config) { rugged_repo(project.repository).config }
+
+  describe '#execute' do
+    context 'using legacy storage' do
+      let(:project) { create(:project, :repository, :legacy_storage) }
+      let(:gitlab_shell) { Gitlab::Shell.new }
+      let(:project_storage) { project.send(:storage) }
+
+      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)
+
+        expect(project).to receive(:expire_caches_before_rename)
+
+        described_class.new(project).execute
+      end
+
+      context 'container registry with images' do
+        let(:container_repository) { create(:container_repository) }
+
+        before do
+          stub_container_registry_config(enabled: true)
+          stub_container_registry_tags(repository: :any, tags: ['tag'])
+          project.container_repositories << container_repository
+        end
+
+        it 'raises a RenameFailedError' do
+          expect { described_class.new(project).execute }
+            .to raise_error(described_class::RenameFailedError)
+        end
+      end
+
+      context 'gitlab pages' do
+        before do
+          expect(project_storage).to receive(:rename_repo) { true }
+        end
+
+        it 'moves pages folder to new location' do
+          expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
+
+          described_class.new(project).execute
+        end
+      end
+
+      context 'attachments' do
+        before do
+          expect(project_storage).to receive(:rename_repo) { true }
+        end
+
+        it 'moves uploads folder to new location' do
+          expect_any_instance_of(Gitlab::UploadsTransfer).to receive(:rename_project)
+
+          described_class.new(project).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
+
+        expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
+      end
+    end
+
+    context 'using hashed storage' do
+      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]) }
+      let(:hashed_path) { File.join(hashed_prefix, hash) }
+
+      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'])
+
+        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)
+
+        expect(gitlab_shell).not_to receive(:mv_repository)
+
+        expect_any_instance_of(SystemHooksService)
+          .to receive(:execute_hooks_for)
+          .with(project, :rename)
+
+        expect(project).to receive(:expire_caches_before_rename)
+
+        described_class.new(project).execute
+      end
+
+      context 'container registry with images' do
+        let(:container_repository) { create(:container_repository) }
+
+        before do
+          stub_container_registry_config(enabled: true)
+          stub_container_registry_tags(repository: :any, tags: ['tag'])
+          project.container_repositories << container_repository
+        end
+
+        it 'raises a RenameFailedError' do
+          expect { described_class.new(project).execute }
+            .to raise_error(described_class::RenameFailedError)
+        end
+      end
+
+      context 'gitlab pages' do
+        it 'moves pages folder to new location' do
+          expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project)
+
+          described_class.new(project).execute
+        end
+      end
+
+      context 'attachments' do
+        it 'keeps uploads folder location unchanged' do
+          expect_any_instance_of(Gitlab::UploadsTransfer).not_to receive(:rename_project)
+
+          described_class.new(project).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
+
+            described_class.new(project).execute
+          end
+        end
+      end
+
+      it 'updates project full path in .git/config' do
+        described_class.new(project).execute
+
+        expect(rugged_config['gitlab.fullpath']).to eq(project.full_path)
+      end
+    end
+  end
+end