diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-12-20 17:22:11 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-12-20 17:22:11 +0300 |
commit | 0c872e02b2c822e3397515ec324051ff540f0cd5 (patch) | |
tree | ce2fb6ce7030e4dad0f4118d21ab6453e5938cdd /spec/services/projects | |
parent | f7e05a6853b12f02911494c4b3fe53d9540d74fc (diff) |
Add latest changes from gitlab-org/gitlab@15-7-stable-eev15.7.0-rc42
Diffstat (limited to 'spec/services/projects')
19 files changed, 386 insertions, 172 deletions
diff --git a/spec/services/projects/after_rename_service_spec.rb b/spec/services/projects/after_rename_service_spec.rb index edf4bbe0f7f..72bb0adbf56 100644 --- a/spec/services/projects/after_rename_service_spec.rb +++ b/spec/services/projects/after_rename_service_spec.rb @@ -9,6 +9,16 @@ RSpec.describe Projects::AfterRenameService do let!(:full_path_before_rename) { project.full_path } let!(:path_after_rename) { "#{project.path}-renamed" } let!(:full_path_after_rename) { "#{project.full_path}-renamed" } + let!(:repo_before_rename) { project.repository.raw } + let!(:wiki_repo_before_rename) { project.wiki.repository.raw } + + let(:repo_after_rename) do + Gitlab::Git::Repository.new(project.repository_storage, "#{full_path_after_rename}.git", nil, nil) + end + + let(:wiki_repo_after_rename) do + Gitlab::Git::Repository.new(project.repository_storage, "#{full_path_after_rename}.wiki.git", nil, nil) + end describe '#execute' do context 'using legacy storage' do @@ -35,13 +45,15 @@ RSpec.describe Projects::AfterRenameService do .to receive(:rename_project) .with(path_before_rename, path_after_rename, project.namespace.full_path) - expect_repository_exist("#{full_path_before_rename}.git") - expect_repository_exist("#{full_path_before_rename}.wiki.git") + expect(repo_before_rename).to exist + expect(wiki_repo_before_rename).to exist service_execute - expect_repository_exist("#{full_path_after_rename}.git") - expect_repository_exist("#{full_path_after_rename}.wiki.git") + expect(repo_before_rename).not_to exist + expect(wiki_repo_before_rename).not_to exist + expect(repo_after_rename).to exist + expect(wiki_repo_after_rename).to exist end context 'container registry with images' do @@ -212,13 +224,4 @@ RSpec.describe Projects::AfterRenameService do 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( - TestEnv.storage_dir_exists?( - project.repository_storage, - full_path_with_extension - ) - ).to be_truthy - end end diff --git a/spec/services/projects/container_repository/destroy_service_spec.rb b/spec/services/projects/container_repository/destroy_service_spec.rb index 20e75d94e05..0ec0aecaa04 100644 --- a/spec/services/projects/container_repository/destroy_service_spec.rb +++ b/spec/services/projects/container_repository/destroy_service_spec.rb @@ -16,7 +16,7 @@ RSpec.describe Projects::ContainerRepository::DestroyService do let!(:repository) { create(:container_repository, :root, project: project) } it 'does not delete a repository' do - expect { subject.execute(repository) }.not_to change { ContainerRepository.all.count } + expect { subject.execute(repository) }.not_to change { ContainerRepository.count } end end @@ -29,23 +29,62 @@ RSpec.describe Projects::ContainerRepository::DestroyService do let!(:repository) { create(:container_repository, :root, project: project) } before do - stub_container_registry_tags(repository: :any, tags: []) + stub_container_registry_tags(repository: :any, tags: %w[latest stable]) end it 'deletes the repository' do - expect(repository).to receive(:delete_tags!).and_call_original - expect { described_class.new(project, user).execute(repository) }.to change { ContainerRepository.all.count }.by(-1) + expect_cleanup_tags_service_with(container_repository: repository, return_status: :success) + expect { subject.execute(repository) }.to change { ContainerRepository.count }.by(-1) end - context 'when destroy fails' do - it 'set delete_status' do + it 'sends disable_timeout = true as part of the params as default' do + expect_cleanup_tags_service_with(container_repository: repository, return_status: :success, disable_timeout: true) + expect { subject.execute(repository) }.to change { ContainerRepository.count }.by(-1) + end + + it 'sends disable_timeout = false as part of the params if it is set to false' do + expect_cleanup_tags_service_with(container_repository: repository, return_status: :success, disable_timeout: false) + expect { subject.execute(repository, disable_timeout: false) }.to change { ContainerRepository.count }.by(-1) + end + + context 'when deleting the tags fails' do + it 'sets status as deleted_failed' do + expect_cleanup_tags_service_with(container_repository: repository, return_status: :error) + allow(Gitlab::AppLogger).to receive(:error).and_call_original + + subject.execute(repository) + + expect(repository).to be_delete_failed + expect(Gitlab::AppLogger).to have_received(:error) + .with("Container repository with ID: #{repository.id} and path: #{repository.path} failed with message: error in deleting tags") + end + end + + context 'when destroying the repository fails' do + it 'sets status as deleted_failed' do + expect_cleanup_tags_service_with(container_repository: repository, return_status: :success) allow(repository).to receive(:destroy).and_return(false) + allow(repository.errors).to receive(:full_messages).and_return(['Error 1', 'Error 2']) + allow(Gitlab::AppLogger).to receive(:error).and_call_original subject.execute(repository) expect(repository).to be_delete_failed + expect(Gitlab::AppLogger).to have_received(:error) + .with("Container repository with ID: #{repository.id} and path: #{repository.path} failed with message: Error 1. Error 2") end end + + def expect_cleanup_tags_service_with(container_repository:, return_status:, disable_timeout: true) + delete_tags_service = instance_double(Projects::ContainerRepository::CleanupTagsService) + + expect(Projects::ContainerRepository::CleanupTagsService).to receive(:new).with( + container_repository: container_repository, + params: described_class::CLEANUP_TAGS_SERVICE_PARAMS.merge('disable_timeout' => disable_timeout) + ).and_return(delete_tags_service) + + expect(delete_tags_service).to receive(:execute).and_return(status: return_status) + end end end end diff --git a/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb index 59827ea035e..b06a5709bd5 100644 --- a/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/gitlab/cleanup_tags_service_spec.rb @@ -49,6 +49,9 @@ RSpec.describe Projects::ContainerRepository::Gitlab::CleanupTagsService do it_behaves_like 'when regex matching everything is specified', delete_expectations: [%w[A], %w[Ba Bb], %w[C D], %w[E]] + it_behaves_like 'when regex matching everything is specified and latest is not kept', + delete_expectations: [%w[latest A], %w[Ba Bb], %w[C D], %w[E]] + it_behaves_like 'when delete regex matching specific tags is used' it_behaves_like 'when delete regex matching specific tags is used with overriding allow regex' @@ -97,6 +100,19 @@ RSpec.describe Projects::ContainerRepository::Gitlab::CleanupTagsService do is_expected.to eq(response) end + + context 'when disable_timeout is set to true' do + let(:params) do + { 'name_regex_delete' => '.*', 'disable_timeout' => true } + end + + it 'does not check if it timed out' do + expect(service).not_to receive(:timeout?) + end + + it_behaves_like 'when regex matching everything is specified', + delete_expectations: [%w[A], %w[Ba Bb], %w[C D], %w[E]] + end end end diff --git a/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb index 8d8907119f0..f03912dba80 100644 --- a/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb +++ b/spec/services/projects/container_repository/gitlab/delete_tags_service_spec.rb @@ -24,6 +24,10 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do context 'with tags to delete' do let(:timeout) { 10 } + before do + stub_application_setting(container_registry_delete_tags_service_timeout: timeout) + end + it_behaves_like 'deleting tags' it 'succeeds when tag delete returns 404' do @@ -48,10 +52,6 @@ RSpec.describe Projects::ContainerRepository::Gitlab::DeleteTagsService do end end - before do - stub_application_setting(container_registry_delete_tags_service_timeout: timeout) - end - context 'with timeout' do context 'set to a valid value' do before do diff --git a/spec/services/projects/container_repository/third_party/cleanup_tags_service_spec.rb b/spec/services/projects/container_repository/third_party/cleanup_tags_service_spec.rb index 2d034d577ac..7227834b131 100644 --- a/spec/services/projects/container_repository/third_party/cleanup_tags_service_spec.rb +++ b/spec/services/projects/container_repository/third_party/cleanup_tags_service_spec.rb @@ -51,6 +51,16 @@ RSpec.describe Projects::ContainerRepository::ThirdParty::CleanupTagsService, :c }, supports_caching: true + it_behaves_like 'when regex matching everything is specified and latest is not kept', + delete_expectations: [%w[A Ba Bb C D E latest]], + service_response_extra: { + before_truncate_size: 7, + after_truncate_size: 7, + before_delete_size: 7, + cached_tags_count: 0 + }, + supports_caching: true + it_behaves_like 'when delete regex matching specific tags is used', service_response_extra: { before_truncate_size: 2, diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 9c8aeb5cf7b..f42ab198a04 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -145,6 +145,20 @@ RSpec.describe Projects::CreateService, '#execute' do end end end + + context 'when the passed in namespace is for a bot user' do + let(:bot_user) { create(:user, :project_bot) } + let(:opts) do + { name: project_name, namespace_id: bot_user.namespace.id } + end + + it 'raises an error' do + project = create_project(bot_user, opts) + + expect(project.errors.errors.length).to eq 1 + expect(project.errors.messages[:namespace].first).to eq(("is not valid")) + end + end end describe 'after create actions' do @@ -908,27 +922,6 @@ RSpec.describe Projects::CreateService, '#execute' do end end - it_behaves_like 'measurable service' do - before do - opts.merge!( - current_user: user, - path: 'foo' - ) - end - - let(:base_log_data) do - { - class: Projects::CreateService.name, - current_user: user.name, - project_full_path: "#{user.namespace.full_path}/#{opts[:path]}" - } - end - - after do - create_project(user, opts) - end - end - context 'with specialized project_authorization workers' do let_it_be(:other_user) { create(:user) } let_it_be(:group) { create(:group) } diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index f7f02769f6a..ff2de45661f 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -4,6 +4,7 @@ require 'spec_helper' RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publisher do include ProjectForksHelper + include BatchDestroyDependentAssociationsHelper let_it_be(:user) { create(:user) } @@ -331,8 +332,8 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi context 'when image repository deletion succeeds' do it 'removes tags' do - expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_return(true) + expect_any_instance_of(Projects::ContainerRepository::CleanupTagsService) + .to receive(:execute).and_return({ status: :success }) destroy_project(project, user) end @@ -340,8 +341,8 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi context 'when image repository deletion fails' do it 'raises an exception' do - expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_raise(RuntimeError) + expect_any_instance_of(Projects::ContainerRepository::CleanupTagsService) + .to receive(:execute).and_raise(RuntimeError) expect(destroy_project(project, user)).to be false end @@ -548,6 +549,30 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi end end + context 'associations destoyed in batches' do + let!(:merge_request) { create(:merge_request, source_project: project) } + let!(:issue) { create(:issue, project: project) } + let!(:label) { create(:label, project: project) } + + it 'destroys the associations marked as `dependent: :destroy`, in batches' do + query_recorder = ActiveRecord::QueryRecorder.new do + destroy_project(project, user, {}) + end + + expect(project.merge_requests).to be_empty + expect(project.issues).to be_empty + expect(project.labels).to be_empty + + expected_queries = [ + delete_in_batches_regexps(:merge_requests, :target_project_id, project, [merge_request]), + delete_in_batches_regexps(:issues, :project_id, project, [issue]), + delete_in_batches_regexps(:labels, :project_id, project, [label]) + ].flatten + + expect(query_recorder.log).to include(*expected_queries) + end + end + def destroy_project(project, user, params = {}) described_class.new(project, user, params).public_send(async ? :async_execute : :execute) end diff --git a/spec/services/projects/download_service_spec.rb b/spec/services/projects/download_service_spec.rb index 7d4fce814f5..f158b11a9fa 100644 --- a/spec/services/projects/download_service_spec.rb +++ b/spec/services/projects/download_service_spec.rb @@ -21,8 +21,8 @@ RSpec.describe Projects::DownloadService do context 'for URLs that are on the whitelist' do before do # `ssrf_filter` resolves the hostname. See https://github.com/carrierwaveuploader/carrierwave/commit/91714adda998bc9e8decf5b1f5d260d808761304 - stub_request(:get, %r{http://[\d\.]+/rails_sample.jpg}).to_return(body: File.read(Rails.root + 'spec/fixtures/rails_sample.jpg')) - stub_request(:get, %r{http://[\d\.]+/doc_sample.txt}).to_return(body: File.read(Rails.root + 'spec/fixtures/doc_sample.txt')) + stub_request(:get, %r{http://[\d.]+/rails_sample.jpg}).to_return(body: File.read(Rails.root + 'spec/fixtures/rails_sample.jpg')) + stub_request(:get, %r{http://[\d.]+/doc_sample.txt}).to_return(body: File.read(Rails.root + 'spec/fixtures/doc_sample.txt')) end context 'an image file' do diff --git a/spec/services/projects/import_export/export_service_spec.rb b/spec/services/projects/import_export/export_service_spec.rb index 285687505e9..2c1ebe27014 100644 --- a/spec/services/projects/import_export/export_service_spec.rb +++ b/spec/services/projects/import_export/export_service_spec.rb @@ -216,24 +216,9 @@ RSpec.describe Projects::ImportExport::ExportService do it 'fails' do expected_message = "User with ID: %s does not have required permissions for Project: %s with ID: %s" % - [another_user.id, project.name, project.id] + [another_user.id, project.name, project.id] expect { service.execute }.to raise_error(Gitlab::ImportExport::Error).with_message(expected_message) end end - - it_behaves_like 'measurable service' do - let(:base_log_data) do - { - class: described_class.name, - current_user: user.name, - project_full_path: project.full_path, - file_path: shared.export_path - } - end - - after do - service.execute(after_export_strategy) - end - end end end diff --git a/spec/services/projects/import_export/parallel_export_service_spec.rb b/spec/services/projects/import_export/parallel_export_service_spec.rb new file mode 100644 index 00000000000..b9f2867077c --- /dev/null +++ b/spec/services/projects/import_export/parallel_export_service_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Projects::ImportExport::ParallelExportService, feature_category: :importers do + let_it_be(:user) { create(:user) } + + let(:export_job) { create(:project_export_job) } + let(:after_export_strategy) { Gitlab::ImportExport::AfterExportStrategies::DownloadNotificationStrategy.new } + let(:project) { export_job.project } + + before do + allow_next_instance_of(Gitlab::ImportExport::Project::ExportedRelationsMerger) do |saver| + allow(saver).to receive(:save).and_return(true) + end + + allow_next_instance_of(Gitlab::ImportExport::VersionSaver) do |saver| + allow(saver).to receive(:save).and_return(true) + end + end + + describe '#execute' do + subject(:service) { described_class.new(export_job, user, after_export_strategy) } + + it 'creates a project export archive file' do + expect(Gitlab::ImportExport::Saver).to receive(:save) + .with(exportable: project, shared: project.import_export_shared) + + service.execute + end + + it 'logs export progress' do + allow(Gitlab::ImportExport::Saver).to receive(:save).and_return(true) + + logger = service.instance_variable_get(:@logger) + messages = [ + 'Parallel project export started', + 'Parallel project export - Gitlab::ImportExport::VersionSaver saver started', + 'Parallel project export - Gitlab::ImportExport::Project::ExportedRelationsMerger saver started', + 'Parallel project export finished successfully' + ] + messages.each do |message| + expect(logger).to receive(:info).ordered.with(hash_including(message: message)) + end + + service.execute + end + + it 'executes after export stragegy on export success' do + allow(Gitlab::ImportExport::Saver).to receive(:save).and_return(true) + + expect(after_export_strategy).to receive(:execute) + + service.execute + end + + it 'ensures files are cleaned up' do + shared = project.import_export_shared + FileUtils.mkdir_p(shared.archive_path) + FileUtils.mkdir_p(shared.export_path) + + allow(Gitlab::ImportExport::Saver).to receive(:save).and_raise(StandardError) + + expect { service.execute }.to raise_error(StandardError) + + expect(File.exist?(shared.export_path)).to eq(false) + expect(File.exist?(shared.archive_path)).to eq(false) + end + + context 'when export fails' do + it 'notifies the error to the user' do + allow(Gitlab::ImportExport::Saver).to receive(:save).and_return(false) + + allow(project.import_export_shared).to receive(:errors).and_return(['Error']) + + expect_next_instance_of(NotificationService) do |instance| + expect(instance).to receive(:project_not_exported).with(project, user, ['Error']) + end + + service.execute + end + end + + context 'when after export stragegy fails' do + it 'notifies the error to the user' do + allow(Gitlab::ImportExport::Saver).to receive(:save).and_return(true) + allow(after_export_strategy).to receive(:execute).and_return(false) + allow(project.import_export_shared).to receive(:errors).and_return(['Error']) + + expect_next_instance_of(NotificationService) do |instance| + expect(instance).to receive(:project_not_exported).with(project, user, ['Error']) + end + + service.execute + end + end + end +end diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index b3f8980a7bd..bb11b2e617e 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -419,25 +419,5 @@ RSpec.describe Projects::ImportService do end end end - - it_behaves_like 'measurable service' do - let(:base_log_data) do - { - class: described_class.name, - current_user: user.name, - project_full_path: project.full_path, - import_type: project.import_type, - file_path: project.import_source - } - end - - before do - project.import_type = 'github' - end - - after do - subject.execute - end - end end end diff --git a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb index d472d6493c3..80b3c4d0403 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_link_list_service_spec.rb @@ -39,7 +39,7 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do ] end - subject { described_class.new(project, remote_uri: remote_uri) } + subject(:service) { described_class.new(project, remote_uri: remote_uri) } before do allow(project).to receive(:lfs_enabled?).and_return(true) @@ -48,19 +48,24 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do allow(Gitlab::HTTP).to receive(:post).and_return(response) end - describe '#execute' do - let(:download_objects) { subject.execute(new_oids) } - + describe '#each_link' do it 'retrieves each download link of every non existent lfs object' do - download_objects.each do |lfs_download_object| - expect(lfs_download_object.link).to eq "#{import_url}/gitlab-lfs/objects/#{lfs_download_object.oid}" - end + links = [] + service.each_link(new_oids) { |lfs_download_object| links << lfs_download_object.link } + + expect(links).to contain_exactly( + "#{import_url}/gitlab-lfs/objects/oid1", + "#{import_url}/gitlab-lfs/objects/oid2" + ) end it 'stores headers' do - download_objects.each do |lfs_download_object| - expect(lfs_download_object.headers).to eq(headers) + expected_headers = [] + service.each_link(new_oids) do |lfs_download_object| + expected_headers << lfs_download_object.headers end + + expect(expected_headers).to contain_exactly(headers, headers) end context 'when lfs objects size is larger than the batch size' do @@ -97,10 +102,13 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do stub_successful_request([data[4]]) end - it 'retreives them in batches' do - subject.execute(new_oids).each do |lfs_download_object| + it 'retrieves them in batches' do + checksum = 0 + service.each_link(new_oids) do |lfs_download_object| expect(lfs_download_object.link).to eq "#{import_url}/gitlab-lfs/objects/#{lfs_download_object.oid}" + checksum += 1 end + expect(checksum).to eq new_oids.size end end @@ -127,9 +135,12 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do an_instance_of(error_class), project_id: project.id, batch_size: 5, oids_count: 5 ) - subject.execute(new_oids).each do |lfs_download_object| + checksum = 0 + service.each_link(new_oids) do |lfs_download_object| expect(lfs_download_object.link).to eq "#{import_url}/gitlab-lfs/objects/#{lfs_download_object.oid}" + checksum += 1 end + expect(checksum).to eq new_oids.size end end @@ -153,7 +164,7 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do expect(Gitlab::ErrorTracking).to receive(:track_exception).with( an_instance_of(error_class), project_id: project.id, batch_size: 2, oids_count: 5 ) - expect { subject.execute(new_oids) }.to raise_error(described_class::DownloadLinksError) + expect { service.each_link(new_oids) }.to raise_error(described_class::DownloadLinksError) end end end @@ -165,21 +176,23 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do let(:import_url) { 'http://user:password@www.gitlab.com/demo/repo.git' } it 'adds credentials to the download_link' do - result = subject.execute(new_oids) - - result.each do |lfs_download_object| + checksum = 0 + service.each_link(new_oids) do |lfs_download_object| expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_truthy + checksum += 1 end + expect(checksum).to eq new_oids.size end end context 'when lfs_endpoint does not have any credentials' do it 'does not add any credentials' do - result = subject.execute(new_oids) - - result.each do |lfs_download_object| + checksum = 0 + service.each_link(new_oids) do |lfs_download_object| expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_falsey + checksum += 1 end + expect(checksum).to eq new_oids.size end end end @@ -189,17 +202,18 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do let(:lfs_endpoint) { "#{import_url_with_credentials}/info/lfs/objects/batch" } it 'downloads without any credentials' do - result = subject.execute(new_oids) - - result.each do |lfs_download_object| + checksum = 0 + service.each_link(new_oids) do |lfs_download_object| expect(lfs_download_object.link.starts_with?('http://user:password@')).to be_falsey + checksum += 1 end + expect(checksum).to eq new_oids.size end end end end - describe '#get_download_links' do + describe '#download_links_for' do context 'if request fails' do before do request_timeout_net_response = Net::HTTPRequestTimeout.new('', '', '') @@ -208,7 +222,7 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do end it 'raises an error' do - expect { subject.send(:get_download_links, new_oids) }.to raise_error(described_class::DownloadLinksError) + expect { subject.send(:download_links_for, new_oids) }.to raise_error(described_class::DownloadLinksError) end end @@ -218,7 +232,7 @@ RSpec.describe Projects::LfsPointers::LfsDownloadLinkListService do allow(response).to receive(:body).and_return(body) allow(Gitlab::HTTP).to receive(:post).and_return(response) - expect { subject.send(:get_download_links, new_oids) }.to raise_error(described_class::DownloadLinksError) + expect { subject.send(:download_links_for, new_oids) }.to raise_error(described_class::DownloadLinksError) end end diff --git a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb index 6c7164c5e06..c815ad38843 100644 --- a/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_download_service_spec.rb @@ -108,7 +108,7 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do end end - context 'when file download fails' do + context 'when file downloading response code is not success' do before do allow(Gitlab::HTTP).to receive(:get).and_return(code: 500, 'success?' => false) end @@ -122,6 +122,20 @@ RSpec.describe Projects::LfsPointers::LfsDownloadService do end end + context 'when file downloading request timeout few times' do + before do + allow(Gitlab::HTTP).to receive(:get).and_raise(Net::OpenTimeout) + end + + it_behaves_like 'no lfs object is created' + + it 'retries to get LFS object 3 times before raising exception' do + subject.execute + + expect(Gitlab::HTTP).to have_received(:get).exactly(3).times + end + end + context 'when file download returns a redirect' do let(:redirect_link) { 'http://external-link' } diff --git a/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb index b36b0b8d6b2..32b86ade81e 100644 --- a/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_import_service_spec.rb @@ -5,7 +5,12 @@ RSpec.describe Projects::LfsPointers::LfsImportService do let(:project) { create(:project) } let(:user) { project.creator } let(:import_url) { 'http://www.gitlab.com/demo/repo.git' } - let(:oid_download_links) { { 'oid1' => "#{import_url}/gitlab-lfs/objects/oid1", 'oid2' => "#{import_url}/gitlab-lfs/objects/oid2" } } + let(:oid_download_links) do + [ + { 'oid1' => "#{import_url}/gitlab-lfs/objects/oid1" }, + { 'oid2' => "#{import_url}/gitlab-lfs/objects/oid2" } + ] + end subject { described_class.new(project, user) } @@ -17,7 +22,8 @@ RSpec.describe Projects::LfsPointers::LfsImportService do it 'downloads lfs objects' do service = double expect_next_instance_of(Projects::LfsPointers::LfsObjectDownloadListService) do |instance| - expect(instance).to receive(:execute).and_return(oid_download_links) + expect(instance).to receive(:each_list_item) + .and_yield(oid_download_links[0]).and_yield(oid_download_links[1]) end expect(Projects::LfsPointers::LfsDownloadService).to receive(:new).and_return(service).twice expect(service).to receive(:execute).twice @@ -30,7 +36,7 @@ RSpec.describe Projects::LfsPointers::LfsImportService do context 'when no downloadable lfs object links' do it 'does not call LfsDownloadService' do expect_next_instance_of(Projects::LfsPointers::LfsObjectDownloadListService) do |instance| - expect(instance).to receive(:execute).and_return({}) + expect(instance).to receive(:each_list_item) end expect(Projects::LfsPointers::LfsDownloadService).not_to receive(:new) @@ -44,7 +50,7 @@ RSpec.describe Projects::LfsPointers::LfsImportService do it 'returns error' do error_message = "error message" expect_next_instance_of(Projects::LfsPointers::LfsObjectDownloadListService) do |instance| - expect(instance).to receive(:execute).and_raise(StandardError, error_message) + expect(instance).to receive(:each_list_item).and_raise(StandardError, error_message) end result = subject.execute diff --git a/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb index adcc2b85706..59eb1ed7a29 100644 --- a/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb +++ b/spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb @@ -9,7 +9,13 @@ RSpec.describe Projects::LfsPointers::LfsObjectDownloadListService do let!(:lfs_objects_project) { create_list(:lfs_objects_project, 2, project: project) } let!(:existing_lfs_objects) { LfsObject.pluck(:oid, :size).to_h } let(:oids) { { 'oid1' => 123, 'oid2' => 125 } } - let(:oid_download_links) { { 'oid1' => "#{import_url}/gitlab-lfs/objects/oid1", 'oid2' => "#{import_url}/gitlab-lfs/objects/oid2" } } + let(:oid_download_links) do + [ + { 'oid1' => "#{import_url}/gitlab-lfs/objects/oid1" }, + { 'oid2' => "#{import_url}/gitlab-lfs/objects/oid2" } + ] + end + let(:all_oids) { existing_lfs_objects.merge(oids) } let(:remote_uri) { URI.parse(lfs_endpoint) } @@ -21,17 +27,24 @@ RSpec.describe Projects::LfsPointers::LfsObjectDownloadListService do allow_any_instance_of(Projects::LfsPointers::LfsListService).to receive(:execute).and_return(all_oids) end - describe '#execute' do + describe '#each_list_item' do context 'when no lfs pointer is linked' do before do - allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links) - expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)).and_call_original + allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService) + .to receive(:each_link).with(oids) + .and_yield(oid_download_links[0]) + .and_yield(oid_download_links[1]) end it 'retrieves all lfs pointers in the project repository' do + expect(Projects::LfsPointers::LfsDownloadLinkListService) + .to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)) + .and_call_original expect_any_instance_of(Projects::LfsPointers::LfsListService).to receive(:execute) - subject.execute + checksum = 0 + subject.each_list_item { |lfs_object| checksum += 1 } + expect(checksum).to eq 2 end context 'when no LFS objects exist' do @@ -40,17 +53,23 @@ RSpec.describe Projects::LfsPointers::LfsObjectDownloadListService do end it 'retrieves all LFS objects' do - expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids) + expect(Projects::LfsPointers::LfsDownloadLinkListService) + .to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)).and_call_original + expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService) + .to receive(:each_link).with(all_oids) - subject.execute + subject.each_list_item {} end end context 'when some LFS objects already exist' do it 'retrieves the download links of non-existent objects' do - expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(oids) + expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService) + .to receive(:each_link).with(oids) - subject.execute + checksum = 0 + subject.each_list_item { |lfs_object| checksum += 1 } + expect(checksum).to eq 2 end end end @@ -62,16 +81,15 @@ RSpec.describe Projects::LfsPointers::LfsObjectDownloadListService do context 'when url points to the same import url host' do let(:lfs_endpoint) { "#{import_url}/different_endpoint" } - let(:service) { double } - - before do - allow(service).to receive(:execute) - end + let(:service) { instance_double(Projects::LfsPointers::LfsDownloadLinkListService, each_link: nil) } it 'downloads lfs object using the new endpoint' do - expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: remote_uri).and_return(service) + expect(Projects::LfsPointers::LfsDownloadLinkListService) + .to receive(:new) + .with(project, remote_uri: remote_uri) + .and_return(service) - subject.execute + subject.each_list_item {} end context 'when import url has credentials' do @@ -79,10 +97,14 @@ RSpec.describe Projects::LfsPointers::LfsObjectDownloadListService do it 'adds the credentials to the new endpoint' do expect(Projects::LfsPointers::LfsDownloadLinkListService) - .to receive(:new).with(project, remote_uri: URI.parse("http://user:password@www.gitlab.com/demo/repo.git/different_endpoint")) + .to receive(:new) + .with( + project, + remote_uri: URI.parse("http://user:password@www.gitlab.com/demo/repo.git/different_endpoint") + ) .and_return(service) - subject.execute + subject.each_list_item {} end context 'when url has its own credentials' do @@ -93,7 +115,7 @@ RSpec.describe Projects::LfsPointers::LfsObjectDownloadListService do .to receive(:new).with(project, remote_uri: remote_uri) .and_return(service) - subject.execute + subject.each_list_item {} end end end @@ -105,7 +127,7 @@ RSpec.describe Projects::LfsPointers::LfsObjectDownloadListService do it 'disables lfs from the project' do expect(project.lfs_enabled?).to be_truthy - subject.execute + subject.each_list_item {} expect(project.lfs_enabled?).to be_falsey end @@ -113,7 +135,7 @@ RSpec.describe Projects::LfsPointers::LfsObjectDownloadListService do it 'does not download anything' do expect_any_instance_of(Projects::LfsPointers::LfsListService).not_to receive(:execute) - subject.execute + subject.each_list_item {} end end end diff --git a/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb b/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb index 6a715312097..a3cff345f68 100644 --- a/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb +++ b/spec/services/projects/refresh_build_artifacts_size_statistics_service_spec.rb @@ -28,6 +28,7 @@ RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitl end let(:now) { Time.zone.now } + let(:statistics) { project.statistics } around do |example| freeze_time { example.run } @@ -45,7 +46,7 @@ RSpec.describe Projects::RefreshBuildArtifactsSizeStatisticsService, :clean_gitl end it 'increments the counter attribute by the total size of the current batch of artifacts' do - expect { service.execute }.to change { project.statistics.get_counter_value(:build_artifacts_size) }.to(3) + expect { service.execute }.to change { statistics.counter(:build_artifacts_size).get }.to(3) end it 'updates the last_job_artifact_id to the ID of the last artifact from the batch' do diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 8f505c31c5a..4d75786a4c3 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -263,7 +263,7 @@ RSpec.describe Projects::TransferService do end context 'when transfer fails' do - let!(:original_path) { project_path(project) } + let!(:original_path) { project.repository.relative_path } def attempt_project_transfer(&block) expect do @@ -277,21 +277,11 @@ RSpec.describe Projects::TransferService do expect_any_instance_of(Labels::TransferService).to receive(:execute).and_raise(ActiveRecord::StatementInvalid, "PG ERROR") end - def project_path(project) - Gitlab::GitalyClient::StorageSettings.allow_disk_access do - project.repository.path_to_repo - end - end - - def current_path - project_path(project) - end - it 'rolls back repo location' do attempt_project_transfer expect(project.repository.raw.exists?).to be(true) - expect(original_path).to eq current_path + expect(original_path).to eq project.repository.relative_path end it 'rolls back project full path in gitaly' do @@ -462,6 +452,22 @@ RSpec.describe Projects::TransferService do end end + context 'target namespace belongs to bot user', :enable_admin_mode do + let(:bot) { create(:user, :project_bot) } + let(:target) { bot.namespace } + let(:executor) { create(:user, :admin) } + + it 'does not allow project transfer' do + namespace = project.namespace + + transfer_result = execute_transfer + + expect(transfer_result).to eq false + expect(project.namespace).to eq(namespace) + expect(project.errors[:new_namespace]).to include("You don't have permission to transfer projects into that namespace.") + end + end + context 'when user does not own the project' do let(:project) { create(:project, :repository, :legacy_storage) } diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index a69db3b9970..d908a169898 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -320,10 +320,11 @@ RSpec.describe Projects::UpdatePagesService do end context 'when retrying the job' do + let(:stage) { create(:ci_stage, position: 1_000_000, name: 'deploy', pipeline: pipeline) } let!(:older_deploy_job) do create(:generic_commit_status, :failed, pipeline: pipeline, ref: build.ref, - stage: 'deploy', + ci_stage: stage, name: 'pages:deploy') end @@ -337,13 +338,15 @@ RSpec.describe Projects::UpdatePagesService do expect(execute).to eq(:success) expect(older_deploy_job.reload).to be_retried + expect(deploy_status.ci_stage).to eq(stage) + expect(deploy_status.stage_idx).to eq(stage.position) end end private def deploy_status - GenericCommitStatus.find_by(name: 'pages:deploy') + GenericCommitStatus.where(name: 'pages:deploy').last end def execute diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 7d8951bf111..3cda6bc2627 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -1,5 +1,4 @@ # frozen_string_literal: true - require 'spec_helper' RSpec.describe Projects::UpdateService do @@ -303,6 +302,25 @@ RSpec.describe Projects::UpdateService do expect(project.default_branch).to eq 'master' expect(project.previous_default_branch).to be_nil end + + context 'when repository has an ambiguous branch named "HEAD"' do + before do + allow(project.repository.raw).to receive(:write_ref).and_return(false) + allow(project.repository).to receive(:branch_names) { %w[fix master main HEAD] } + end + + it 'returns an error to the user' do + result = update_project(project, admin, default_branch: 'fix') + + expect(result).to include(status: :error) + expect(result[:message]).to include("Could not set the default branch. Do you have a branch named 'HEAD' in your repository?") + + project.reload + + expect(project.default_branch).to eq 'master' + expect(project.previous_default_branch).to be_nil + end + end end context 'when we update project but not enabling a wiki' do @@ -422,25 +440,6 @@ RSpec.describe Projects::UpdateService do expect(feature.feature_flags_access_level).not_to eq(ProjectFeature::DISABLED) expect(feature.environments_access_level).not_to eq(ProjectFeature::DISABLED) end - - context 'when split_operations_visibility_permissions feature is disabled' do - before do - stub_feature_flags(split_operations_visibility_permissions: false) - end - - it 'syncs the changes to the related fields' do - result = update_project(project, user, project_feature_attributes: feature_params) - - expect(result).to eq({ status: :success }) - feature = project.project_feature - - expect(feature.operations_access_level).to eq(ProjectFeature::DISABLED) - expect(feature.monitor_access_level).to eq(ProjectFeature::DISABLED) - expect(feature.infrastructure_access_level).to eq(ProjectFeature::DISABLED) - expect(feature.feature_flags_access_level).to eq(ProjectFeature::DISABLED) - expect(feature.environments_access_level).to eq(ProjectFeature::DISABLED) - end - end end context 'when updating a project that contains container images' do |