diff options
10 files changed, 81 insertions, 48 deletions
diff --git a/app/models/storage/hashed_project.rb b/app/models/storage/hashed_project.rb index 911fb7e9ce9..f5d0d6fab3b 100644 --- a/app/models/storage/hashed_project.rb +++ b/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 diff --git a/app/models/storage/legacy_project.rb b/app/models/storage/legacy_project.rb index 9f6f19acb41..76ac5c13c18 100644 --- a/app/models/storage/legacy_project.rb +++ b/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 diff --git a/app/services/projects/after_rename_service.rb b/app/services/projects/after_rename_service.rb index 9ace1f85336..c3cd9d1ea4a 100644 --- a/app/services/projects/after_rename_service.rb +++ b/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 diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index d5a3a4043f3..6856009b395 100644 --- a/app/services/projects/update_service.rb +++ b/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? diff --git a/changelogs/unreleased/56636-hashed-storage-afterrenameservice.yml b/changelogs/unreleased/56636-hashed-storage-afterrenameservice.yml new file mode 100644 index 00000000000..1f808850554 --- /dev/null +++ b/changelogs/unreleased/56636-hashed-storage-afterrenameservice.yml @@ -0,0 +1,5 @@ +--- +title: 'Hashed Storage: `AfterRenameService` was receiving the wrong `old_path` under some circunstances' +merge_request: 24526 +author: +type: fixed diff --git a/db/post_migrate/20161221153951_rename_reserved_project_names.rb b/db/post_migrate/20161221153951_rename_reserved_project_names.rb index 50e1c8449ba..32579256299 100644 --- a/db/post_migrate/20161221153951_rename_reserved_project_names.rb +++ b/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 diff --git a/db/post_migrate/20170313133418_rename_more_reserved_project_names.rb b/db/post_migrate/20170313133418_rename_more_reserved_project_names.rb index bef669b459d..85c97e3687e 100644 --- a/db/post_migrate/20170313133418_rename_more_reserved_project_names.rb +++ b/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 diff --git a/spec/migrations/rename_more_reserved_project_names_spec.rb b/spec/migrations/rename_more_reserved_project_names_spec.rb index baf16c2ce53..80ae209e9d1 100644 --- a/spec/migrations/rename_more_reserved_project_names_spec.rb +++ b/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 diff --git a/spec/migrations/rename_reserved_project_names_spec.rb b/spec/migrations/rename_reserved_project_names_spec.rb index 7818aa0d560..93e5c032287 100644 --- a/spec/migrations/rename_reserved_project_names_spec.rb +++ b/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 diff --git a/spec/services/projects/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb index 7f272ffcd7b..bc5366a3339 100644 --- a/spec/services/projects/after_rename_service_spec.rb +++ b/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 |