Skip to content

G
gitlab-ce
Commit 4b9c17f1 authored by Yorick Peterse's avatar Yorick Peterse
Browse files
Options

Move Project#rename_repo to a service class 

This moves the logic of Project#rename_repo and all methods _only_ used
by this method into a new service class: Projects::AfterRenameService.
By moving this code into a separate service class we can more easily
refactor it, and we also get rid of some RuboCop "disable" statements
automatically.

During the refactoring of this code, I removed most of the explicit
logging using Gitlab::AppLogger. The data that was logged would not be
useful when debugging renaming issues, as it does not add any value on
top of data provided by users.

I also removed a variety of comments that either mentioned something the
code does in literal form, or contained various grammatical errors.
Instead we now resort to more clearly named methods, removing the need
for code comments.

This method was chosen based on analysis in
https://gitlab.com/gitlab-org/release/framework/issues/28. In this issue
we determined this method has seen a total of 293 lines being changed in
it. We also noticed that RuboCop determined the ABC size
(https://www.softwarerenovation.com/ABCMetric.pdf) was too great.
parent da1a2bd6
Hide whitespace changes
Inline Side-by-side
Showing with 368 additions and 247 deletions
app/models/project.rb
View file @ 4b9c17f1
...@@ -1640,34 +1640,6 @@ class Project < ActiveRecord::Base ...@@ -1640,34 +1640,6 @@ class Project < ActiveRecord::Base
end end
# rubocop: enable CodeReuse/ServiceClass # 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) def write_repository_config(gl_full_path: full_path)
# We'd need to keep track of project full path otherwise directory tree # We'd need to keep track of project full path otherwise directory tree
# created with hashed storage enabled cannot be usefully imported using # created with hashed storage enabled cannot be usefully imported using
...@@ -2096,51 +2068,6 @@ class Project < ActiveRecord::Base ...@@ -2096,51 +2068,6 @@ class Project < ActiveRecord::Base
auto_cancel_pending_pipelines == 'enabled' auto_cancel_pending_pipelines == 'enabled'
end 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 def storage
@storage ||= @storage ||=
if hashed_storage?(:repository) if hashed_storage?(:repository)
...@@ -2150,6 +2077,12 @@ class Project < ActiveRecord::Base ...@@ -2150,6 +2077,12 @@ class Project < ActiveRecord::Base
end end
end end
def storage_upgradable?
storage_version != LATEST_STORAGE_VERSION
end
private
def use_hashed_storage def use_hashed_storage
if self.new_record? && Gitlab::CurrentSettings.hashed_storage_enabled if self.new_record? && Gitlab::CurrentSettings.hashed_storage_enabled
self.storage_version = LATEST_STORAGE_VERSION self.storage_version = LATEST_STORAGE_VERSION
......
app/services/projects/after_rename_service.rb 0 → 100644
View file @ 4b9c17f1
# 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
View file @ 4b9c17f1
...@@ -67,7 +67,7 @@ module Projects ...@@ -67,7 +67,7 @@ module Projects
end end
if project.previous_changes.include?('path') if project.previous_changes.include?('path')
project.rename_repo AfterRenameService.new(project).execute
else else
system_hook_service.execute_hooks_for(project, :update) system_hook_service.execute_hooks_for(project, :update)
end end
......
db/post_migrate/20161221153951_rename_reserved_project_names.rb
View file @ 4b9c17f1
...@@ -113,7 +113,9 @@ class RenameReservedProjectNames < ActiveRecord::Migration ...@@ -113,7 +113,9 @@ class RenameReservedProjectNames < ActiveRecord::Migration
begin begin
# Because project path update is quite complex operation we can't safely # 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 # 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 rescue Exception => e # rubocop: disable Lint/RescueException
Rails.logger.error "Exception when renaming project #{id}: #{e.message}" Rails.logger.error "Exception when renaming project #{id}: #{e.message}"
end end
...@@ -123,6 +125,6 @@ class RenameReservedProjectNames < ActiveRecord::Migration ...@@ -123,6 +125,6 @@ class RenameReservedProjectNames < ActiveRecord::Migration
def rename_project_row(project, path) def rename_project_row(project, path)
project.respond_to?(:update_attributes) && project.respond_to?(:update_attributes) &&
project.update(path: path) && project.update(path: path) &&
project.respond_to?(:rename_repo) defined?(Projects::AfterRenameService)
end end
end end
db/post_migrate/20170313133418_rename_more_reserved_project_names.rb
View file @ 4b9c17f1
...@@ -55,7 +55,9 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration ...@@ -55,7 +55,9 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration
begin begin
# Because project path update is quite complex operation we can't safely # 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 # 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 rescue Exception => e # rubocop: disable Lint/RescueException
Rails.logger.error "Exception when renaming project #{id}: #{e.message}" Rails.logger.error "Exception when renaming project #{id}: #{e.message}"
end end
...@@ -65,6 +67,6 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration ...@@ -65,6 +67,6 @@ class RenameMoreReservedProjectNames < ActiveRecord::Migration
def rename_project_row(project, path) def rename_project_row(project, path)
project.respond_to?(:update_attributes) && project.respond_to?(:update_attributes) &&
project.update(path: path) && project.update(path: path) &&
project.respond_to?(:rename_repo) defined?(Projects::AfterRenameService)
end end
end end
spec/migrations/rename_more_reserved_project_names_spec.rb
View file @ 4b9c17f1
...@@ -31,7 +31,16 @@ describe RenameMoreReservedProjectNames, :delete do ...@@ -31,7 +31,16 @@ describe RenameMoreReservedProjectNames, :delete do
context 'when exception is raised during rename' do context 'when exception is raised during rename' do
before 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 end
it 'captures exception from project rename' do it 'captures exception from project rename' do
......
spec/migrations/rename_reserved_project_names_spec.rb
View file @ 4b9c17f1
...@@ -35,7 +35,16 @@ describe RenameReservedProjectNames, :migration, schema: :latest do ...@@ -35,7 +35,16 @@ describe RenameReservedProjectNames, :migration, schema: :latest do
context 'when exception is raised during rename' do context 'when exception is raised during rename' do
before 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 end
it 'captures exception from project rename' do it 'captures exception from project rename' do
......
spec/models/project_spec.rb
View file @ 4b9c17f1
...@@ -2956,88 +2956,6 @@ describe Project do ...@@ -2956,88 +2956,6 @@ describe Project do
end end
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 describe '#pages_path' do
it 'returns a path where pages are stored' 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)) expect(project.pages_path).to eq(File.join(Settings.pages.path, project.namespace.full_path, project.path))
...@@ -3128,91 +3046,6 @@ describe Project do ...@@ -3128,91 +3046,6 @@ describe Project do
end end
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 describe '#pages_path' do
it 'returns a path where pages are stored' 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)) 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 0 → 100644
View file @ 4b9c17f1
# 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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7