diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-02-20 16:49:51 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-02-20 16:49:51 +0300 |
commit | 71786ddc8e28fbd3cb3fcc4b3ff15e5962a1c82e (patch) | |
tree | 6a2d93ef3fb2d353bb7739e4b57e6541f51cdd71 /spec/services/projects | |
parent | a7253423e3403b8c08f8a161e5937e1488f5f407 (diff) |
Add latest changes from gitlab-org/gitlab@15-9-stable-eev15.9.0-rc42
Diffstat (limited to 'spec/services/projects')
10 files changed, 228 insertions, 68 deletions
diff --git a/spec/services/projects/container_repository/destroy_service_spec.rb b/spec/services/projects/container_repository/destroy_service_spec.rb index 0ec0aecaa04..fed1d13daa5 100644 --- a/spec/services/projects/container_repository/destroy_service_spec.rb +++ b/spec/services/projects/container_repository/destroy_service_spec.rb @@ -5,86 +5,131 @@ require 'spec_helper' RSpec.describe Projects::ContainerRepository::DestroyService do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :private) } + let_it_be(:params) { {} } - subject { described_class.new(project, user) } + subject { described_class.new(project, user, params) } before do stub_container_registry_config(enabled: true) end - context 'when user does not have access to registry' do - let!(:repository) { create(:container_repository, :root, project: project) } + shared_examples 'returning an error status with message' do |error_message| + it 'returns an error status' do + response = subject.execute(repository) - it 'does not delete a repository' do - expect { subject.execute(repository) }.not_to change { ContainerRepository.count } + expect(response).to include(status: :error, message: error_message) end end - context 'when user has access to registry' do + shared_examples 'executing with permissions' do + let_it_be_with_refind(:repository) { create(:container_repository, :root, project: project) } + before do - project.add_developer(user) + stub_container_registry_tags(repository: :any, tags: %w[latest stable]) end - context 'when root container repository exists' do - let!(:repository) { create(:container_repository, :root, project: project) } + it 'deletes the repository' do + expect_cleanup_tags_service_with(container_repository: repository, return_status: :success) + expect { subject.execute(repository) }.to change { ContainerRepository.count }.by(-1) + end + + 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 before do - stub_container_registry_tags(repository: :any, tags: %w[latest stable]) + expect_cleanup_tags_service_with(container_repository: repository, return_status: :error) + allow(Gitlab::AppLogger).to receive(:error).and_call_original end - it 'deletes the repository' do - expect_cleanup_tags_service_with(container_repository: repository, return_status: :success) - expect { subject.execute(repository) }.to change { ContainerRepository.count }.by(-1) - end + it 'sets status as deleted_failed' do + subject.execute(repository) - 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) + expect(repository).to be_delete_failed 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 + it 'logs the error' do + subject.execute(repository) - 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 + 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 - subject.execute(repository) + it_behaves_like 'returning an error status with message', 'Deletion failed for container repository' + end - 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 + context 'when destroying the repository fails' do + before 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 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 + it 'sets status as deleted_failed' do + subject.execute(repository) + + expect(repository).to be_delete_failed + end - subject.execute(repository) + it 'logs the error' do + 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 + 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 - def expect_cleanup_tags_service_with(container_repository:, return_status:, disable_timeout: true) - delete_tags_service = instance_double(Projects::ContainerRepository::CleanupTagsService) + it_behaves_like 'returning an error status with message', 'Deletion failed for container repository' + end + end + + context 'when user has access to registry' do + before do + project.add_developer(user) + end - 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) + it_behaves_like 'executing with permissions' + end - expect(delete_tags_service).to receive(:execute).and_return(status: return_status) - end + context 'when user does not have access to registry' do + let_it_be(:repository) { create(:container_repository, :root, project: project) } + + it 'does not delete a repository' do + expect { subject.execute(repository) }.not_to change { ContainerRepository.count } end + + it_behaves_like 'returning an error status with message', 'Unauthorized access' + end + + context 'when called during project deletion' do + let(:user) { nil } + let(:params) { { skip_permission_check: true } } + + it_behaves_like 'executing with permissions' + end + + context 'when there is no user' do + let(:user) { nil } + let(:repository) { create(:container_repository, :root, project: project) } + + it_behaves_like 'returning an error status with message', 'Unauthorized access' + 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 diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index f85a8eda7ee..e435db4efa6 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -163,7 +163,7 @@ RSpec.describe Projects::CreateService, '#execute', feature_category: :projects describe 'after create actions' do it 'invalidate personal_projects_count caches' do - expect(user).to receive(:invalidate_personal_projects_count) + expect(Rails.cache).to receive(:delete).with(['users', user.id, 'personal_projects_count']) create_project(user, opts) end @@ -947,6 +947,8 @@ RSpec.describe Projects::CreateService, '#execute', feature_category: :projects end it 'schedules authorization update for users with access to group', :sidekiq_inline do + stub_feature_flags(do_not_run_safety_net_auth_refresh_jobs: false) + expect(AuthorizedProjectsWorker).not_to( receive(:bulk_perform_async) ) diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index ff2de45661f..0689a65c2f4 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publisher do +RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publisher, feature_category: :projects do include ProjectForksHelper include BatchDestroyDependentAssociationsHelper @@ -151,10 +151,22 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi it_behaves_like 'deleting the project' - it 'invalidates personal_project_count cache' do - expect(user).to receive(:invalidate_personal_projects_count) + context 'personal projects count cache' do + context 'when the executor is the creator of the project itself' do + it 'invalidates personal_project_count cache of the the owner of the personal namespace' do + expect(user).to receive(:invalidate_personal_projects_count) - destroy_project(project, user, {}) + destroy_project(project, user, {}) + end + end + + context 'when the executor is the instance administrator', :enable_admin_mode do + it 'invalidates personal_project_count cache of the the owner of the personal namespace' do + expect(user).to receive(:invalidate_personal_projects_count) + + destroy_project(project, create(:admin), {}) + end + end end context 'with running pipelines' do @@ -331,18 +343,30 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi end context 'when image repository deletion succeeds' do - it 'removes tags' do - expect_any_instance_of(Projects::ContainerRepository::CleanupTagsService) - .to receive(:execute).and_return({ status: :success }) + it 'returns true' do + expect_next_instance_of(Projects::ContainerRepository::CleanupTagsService) do |instance| + expect(instance).to receive(:execute).and_return(status: :success) + end - destroy_project(project, user) + expect(destroy_project(project, user)).to be true + end + end + + context 'when image repository deletion raises an error' do + it 'returns false' do + expect_next_instance_of(Projects::ContainerRepository::CleanupTagsService) do |service| + expect(service).to receive(:execute).and_raise(RuntimeError) + end + + expect(destroy_project(project, user)).to be false end end context 'when image repository deletion fails' do - it 'raises an exception' do - expect_any_instance_of(Projects::ContainerRepository::CleanupTagsService) - .to receive(:execute).and_raise(RuntimeError) + it 'returns false' do + expect_next_instance_of(Projects::ContainerRepository::DestroyService) do |service| + expect(service).to receive(:execute).and_return({ status: :error }) + end expect(destroy_project(project, user)).to be false end @@ -369,8 +393,9 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi context 'when image repository tags deletion succeeds' do it 'removes tags' do - expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_return(true) + expect_next_instance_of(Projects::ContainerRepository::DestroyService) do |service| + expect(service).to receive(:execute).and_return({ status: :sucess }) + end destroy_project(project, user) end @@ -378,13 +403,27 @@ RSpec.describe Projects::DestroyService, :aggregate_failures, :event_store_publi context 'when image repository tags deletion fails' do it 'raises an exception' do - expect_any_instance_of(ContainerRepository) - .to receive(:delete_tags!).and_return(false) + expect_next_instance_of(Projects::ContainerRepository::DestroyService) do |service| + expect(service).to receive(:execute).and_return({ status: :error }) + end expect(destroy_project(project, user)).to be false end end end + + context 'when there are no tags for legacy root repository' do + before do + stub_container_registry_tags(repository: project.full_path, + tags: []) + end + + it 'does not try to destroy the repository' do + expect(Projects::ContainerRepository::DestroyService).not_to receive(:new) + + destroy_project(project, user) + end + end end context 'for a forked project with LFS objects' do diff --git a/spec/services/projects/group_links/create_service_spec.rb b/spec/services/projects/group_links/create_service_spec.rb index 65d3085a850..eae898b4f68 100644 --- a/spec/services/projects/group_links/create_service_spec.rb +++ b/spec/services/projects/group_links/create_service_spec.rb @@ -58,6 +58,8 @@ RSpec.describe Projects::GroupLinks::CreateService, '#execute' do end it 'schedules authorization update for users with access to group' do + stub_feature_flags(do_not_run_safety_net_auth_refresh_jobs: false) + expect(AuthorizedProjectsWorker).not_to( receive(:bulk_perform_async) ) diff --git a/spec/services/projects/group_links/destroy_service_spec.rb b/spec/services/projects/group_links/destroy_service_spec.rb index 5d07fd52230..89865d6bc3b 100644 --- a/spec/services/projects/group_links/destroy_service_spec.rb +++ b/spec/services/projects/group_links/destroy_service_spec.rb @@ -28,6 +28,8 @@ RSpec.describe Projects::GroupLinks::DestroyService, '#execute' do end it 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' do + stub_feature_flags(do_not_run_safety_net_auth_refresh_jobs: false) + expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( receive(:bulk_perform_in) .with(1.hour, diff --git a/spec/services/projects/group_links/update_service_spec.rb b/spec/services/projects/group_links/update_service_spec.rb index 20616890ebd..1acbb770763 100644 --- a/spec/services/projects/group_links/update_service_spec.rb +++ b/spec/services/projects/group_links/update_service_spec.rb @@ -42,6 +42,8 @@ RSpec.describe Projects::GroupLinks::UpdateService, '#execute' do end it 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' do + stub_feature_flags(do_not_run_safety_net_auth_refresh_jobs: false) + expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( receive(:bulk_perform_in) .with(1.hour, diff --git a/spec/services/projects/import_export/export_service_spec.rb b/spec/services/projects/import_export/export_service_spec.rb index 2c1ebe27014..be059aec697 100644 --- a/spec/services/projects/import_export/export_service_spec.rb +++ b/spec/services/projects/import_export/export_service_spec.rb @@ -2,11 +2,12 @@ require 'spec_helper' -RSpec.describe Projects::ImportExport::ExportService do +RSpec.describe Projects::ImportExport::ExportService, feature_category: :importers do describe '#execute' do let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be_with_reload(:project) { create(:project, group: group) } - let(:project) { create(:project) } let(:shared) { project.import_export_shared } let!(:after_export_strategy) { Gitlab::ImportExport::AfterExportStrategies::DownloadNotificationStrategy.new } @@ -220,5 +221,21 @@ RSpec.describe Projects::ImportExport::ExportService do expect { service.execute }.to raise_error(Gitlab::ImportExport::Error).with_message(expected_message) end end + + it "avoids N+1 when exporting project members" do + group.add_owner(user) + group.add_maintainer(create(:user)) + project.add_maintainer(create(:user)) + + # warm up + service.execute + + control = ActiveRecord::QueryRecorder.new { service.execute } + + group.add_maintainer(create(:user)) + project.add_maintainer(create(:user)) + + expect { service.execute }.not_to exceed_query_limit(control) + end end end diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index 38ab7b6e2ee..97a3b338069 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::ImportService do +RSpec.describe Projects::ImportService, feature_category: :importers do let!(:project) { create(:project) } let(:user) { project.creator } diff --git a/spec/services/projects/protect_default_branch_service_spec.rb b/spec/services/projects/protect_default_branch_service_spec.rb index c8aa421cdd4..9f9e89ff8f8 100644 --- a/spec/services/projects/protect_default_branch_service_spec.rb +++ b/spec/services/projects/protect_default_branch_service_spec.rb @@ -233,6 +233,38 @@ RSpec.describe Projects::ProtectDefaultBranchService do end end + describe '#protected_branch_exists?' do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + + let(:default_branch) { "default-branch" } + + before do + allow(project).to receive(:default_branch).and_return(default_branch) + create(:protected_branch, project: nil, group: group, name: default_branch) + end + + context 'when feature flag `group_protected_branches` disabled' do + before do + stub_feature_flags(group_protected_branches: false) + end + + it 'return false' do + expect(service.protected_branch_exists?).to eq(false) + end + end + + context 'when feature flag `group_protected_branches` enabled' do + before do + stub_feature_flags(group_protected_branches: true) + end + + it 'return true' do + expect(service.protected_branch_exists?).to eq(true) + end + end + end + describe '#default_branch' do it 'returns the default branch of the project' do allow(project) diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 5171836f917..32818535146 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -126,6 +126,12 @@ RSpec.describe Projects::TransferService do expect(project.namespace).to eq(user.namespace) end + it 'invalidates personal_project_count cache of the the owner of the personal namespace' do + expect(user).to receive(:invalidate_personal_projects_count) + + execute_transfer + end + context 'the owner of the namespace does not have a direct membership in the project residing in the group' do it 'creates a project membership record for the owner of the namespace, with OWNER access level, after the transfer' do execute_transfer @@ -161,6 +167,17 @@ RSpec.describe Projects::TransferService do end end + context 'personal namespace -> group', :enable_admin_mode do + let(:executor) { create(:admin) } + + it 'invalidates personal_project_count cache of the the owner of the personal namespace' \ + 'that previously held the project' do + expect(user).to receive(:invalidate_personal_projects_count) + + execute_transfer + end + end + context 'when transfer succeeds' do before do group.add_owner(user) @@ -645,6 +662,8 @@ RSpec.describe Projects::TransferService do end it 'calls AuthorizedProjectUpdate::UserRefreshFromReplicaWorker with a delay to update project authorizations' do + stub_feature_flags(do_not_run_safety_net_auth_refresh_jobs: false) + user_ids = [user.id, member_of_old_group.id, member_of_new_group.id].map { |id| [id] } expect(AuthorizedProjectUpdate::UserRefreshFromReplicaWorker).to( |