diff options
Diffstat (limited to 'spec/requests/api')
78 files changed, 3001 insertions, 649 deletions
diff --git a/spec/requests/api/api_spec.rb b/spec/requests/api/api_spec.rb index 522030652bd..b3e425630e5 100644 --- a/spec/requests/api/api_spec.rb +++ b/spec/requests/api/api_spec.rb @@ -105,9 +105,9 @@ RSpec.describe API::API do it 'logs all application context fields' do allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do - Labkit::Context.current.to_h.tap do |log_context| + Gitlab::ApplicationContext.current.tap do |log_context| expect(log_context).to match('correlation_id' => an_instance_of(String), - 'meta.caller_id' => '/api/:version/projects/:id/issues', + 'meta.caller_id' => 'GET /api/:version/projects/:id/issues', 'meta.remote_ip' => an_instance_of(String), 'meta.project' => project.full_path, 'meta.root_namespace' => project.namespace.full_path, @@ -122,9 +122,9 @@ RSpec.describe API::API do it 'skips fields that do not apply' do allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do - Labkit::Context.current.to_h.tap do |log_context| + Gitlab::ApplicationContext.current.tap do |log_context| expect(log_context).to match('correlation_id' => an_instance_of(String), - 'meta.caller_id' => '/api/:version/users', + 'meta.caller_id' => 'GET /api/:version/users', 'meta.remote_ip' => an_instance_of(String), 'meta.client_id' => an_instance_of(String), 'meta.feature_category' => 'users') @@ -141,7 +141,7 @@ RSpec.describe API::API do let(:component_map) do { "application" => "test", - "endpoint_id" => "/api/:version/users/:id" + "endpoint_id" => "GET /api/:version/users/:id" } end diff --git a/spec/requests/api/applications_spec.rb b/spec/requests/api/applications_spec.rb index ca09f5524ca..959e68e6a0d 100644 --- a/spec/requests/api/applications_spec.rb +++ b/spec/requests/api/applications_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' RSpec.describe API::Applications, :api do let(:admin_user) { create(:user, admin: true) } let(:user) { create(:user, admin: false) } - let!(:application) { create(:application, name: 'another_application', redirect_uri: 'http://other_application.url', scopes: '') } + let!(:application) { create(:application, name: 'another_application', owner: nil, redirect_uri: 'http://other_application.url', scopes: '') } describe 'POST /applications' do context 'authenticated and authorized user' do @@ -143,6 +143,12 @@ RSpec.describe API::Applications, :api do expect(response).to have_gitlab_http_status(:no_content) end + + it 'cannot delete non-existing application' do + delete api("/applications/#{non_existing_record_id}", admin_user) + + expect(response).to have_gitlab_http_status(:not_found) + end end context 'authorized user without authorization' do diff --git a/spec/requests/api/ci/runner/jobs_artifacts_spec.rb b/spec/requests/api/ci/runner/jobs_artifacts_spec.rb index 9369b6aa464..017a12a4a40 100644 --- a/spec/requests/api/ci/runner/jobs_artifacts_spec.rb +++ b/spec/requests/api/ci/runner/jobs_artifacts_spec.rb @@ -127,7 +127,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do authorize_artifacts_with_token_in_params end - it_behaves_like 'API::CI::Runner application context metadata', '/api/:version/jobs/:id/artifacts/authorize' do + it_behaves_like 'API::CI::Runner application context metadata', 'POST /api/:version/jobs/:id/artifacts/authorize' do let(:send_request) { subject } end @@ -180,6 +180,18 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do it_behaves_like 'authorizes local file' end end + + context 'when job does not exist anymore' do + before do + allow(job).to receive(:id).and_return(non_existing_record_id) + end + + it 'returns 403 Forbidden' do + subject + + expect(response).to have_gitlab_http_status(:forbidden) + end + end end end @@ -262,7 +274,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do end describe 'POST /api/v4/jobs/:id/artifacts' do - it_behaves_like 'API::CI::Runner application context metadata', '/api/:version/jobs/:id/artifacts' do + it_behaves_like 'API::CI::Runner application context metadata', 'POST /api/:version/jobs/:id/artifacts' do let(:send_request) do upload_artifacts(file_upload, headers_with_token) end @@ -321,6 +333,18 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do end end + context 'when job does not exist anymore' do + before do + allow(job).to receive(:id).and_return(non_existing_record_id) + end + + it 'returns 403 Forbidden' do + upload_artifacts(file_upload, headers_with_token) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + context 'when job is running' do shared_examples 'successful artifacts upload' do it 'updates successfully' do @@ -784,7 +808,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do describe 'GET /api/v4/jobs/:id/artifacts' do let(:token) { job.token } - it_behaves_like 'API::CI::Runner application context metadata', '/api/:version/jobs/:id/artifacts' do + it_behaves_like 'API::CI::Runner application context metadata', 'GET /api/:version/jobs/:id/artifacts' do let(:send_request) { download_artifact } end @@ -867,6 +891,18 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do end end + context 'when job does not exist anymore' do + before do + allow(job).to receive(:id).and_return(non_existing_record_id) + end + + it 'responds with 403 Forbidden' do + get api("/jobs/#{job.id}/artifacts"), params: { token: token }, headers: headers + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + def download_artifact(params = {}, request_headers = headers) params = params.merge(token: token) job.reload diff --git a/spec/requests/api/ci/runner/jobs_put_spec.rb b/spec/requests/api/ci/runner/jobs_put_spec.rb index b5d2c4608c5..3d5021fba08 100644 --- a/spec/requests/api/ci/runner/jobs_put_spec.rb +++ b/spec/requests/api/ci/runner/jobs_put_spec.rb @@ -36,7 +36,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do job.run! end - it_behaves_like 'API::CI::Runner application context metadata', '/api/:version/jobs/:id' do + it_behaves_like 'API::CI::Runner application context metadata', 'PUT /api/:version/jobs/:id' do let(:send_request) { update_job(state: 'success') } end @@ -278,14 +278,22 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do end end - def update_job(token = job.token, **params) + context 'when job does not exist anymore' do + it 'returns 403 Forbidden' do + update_job(non_existing_record_id, state: 'success', trace: 'BUILD TRACE UPDATED') + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + + def update_job(job_id = job.id, token = job.token, **params) new_params = params.merge(token: token) - put api("/jobs/#{job.id}"), params: new_params + put api("/jobs/#{job_id}"), params: new_params end def update_job_after_time(update_interval = 20.minutes, state = 'running') travel_to(job.updated_at + update_interval) do - update_job(job.token, state: state) + update_job(job.id, job.token, state: state) end end end diff --git a/spec/requests/api/ci/runner/jobs_request_post_spec.rb b/spec/requests/api/ci/runner/jobs_request_post_spec.rb index aced094e219..cf0d8a632f1 100644 --- a/spec/requests/api/ci/runner/jobs_request_post_spec.rb +++ b/spec/requests/api/ci/runner/jobs_request_post_spec.rb @@ -143,7 +143,8 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do context 'when there is a pending job' do let(:expected_job_info) do - { 'name' => job.name, + { 'id' => job.id, + 'name' => job.name, 'stage' => job.stage, 'project_id' => job.project.id, 'project_name' => job.project.name } @@ -490,6 +491,36 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do { 'id' => job.id, 'name' => job.name, 'token' => job.token }, { 'id' => job2.id, 'name' => job2.name, 'token' => job2.token }) end + + describe 'preloading job_artifacts_archive' do + context 'when the feature flag is disabled' do + before do + stub_feature_flags(preload_associations_jobs_request_api_endpoint: false) + end + + it 'queries the ci_job_artifacts table multiple times' do + expect { request_job }.to exceed_all_query_limit(1).for_model(::Ci::JobArtifact) + end + + it 'queries the ci_builds table more than five times' do + expect { request_job }.to exceed_all_query_limit(5).for_model(::Ci::Build) + end + end + + context 'when the feature flag is enabled' do + before do + stub_feature_flags(preload_associations_jobs_request_api_endpoint: true) + end + + it 'queries the ci_job_artifacts table once only' do + expect { request_job }.not_to exceed_all_query_limit(1).for_model(::Ci::JobArtifact) + end + + it 'queries the ci_builds table five times' do + expect { request_job }.not_to exceed_all_query_limit(5).for_model(::Ci::Build) + end + end + end end context 'when pipeline have jobs with artifacts' do diff --git a/spec/requests/api/ci/runner/jobs_trace_spec.rb b/spec/requests/api/ci/runner/jobs_trace_spec.rb index 659cf055023..e077a174b08 100644 --- a/spec/requests/api/ci/runner/jobs_trace_spec.rb +++ b/spec/requests/api/ci/runner/jobs_trace_spec.rb @@ -41,7 +41,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do initial_patch_the_trace end - it_behaves_like 'API::CI::Runner application context metadata', '/api/:version/jobs/:id/trace' do + it_behaves_like 'API::CI::Runner application context metadata', 'PATCH /api/:version/jobs/:id/trace' do let(:send_request) { patch_the_trace } end @@ -210,15 +210,23 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do end context 'when build trace is not being watched' do - it 'returns X-GitLab-Trace-Update-Interval as 30' do + it 'returns the interval in X-GitLab-Trace-Update-Interval' do patch_the_trace expect(response).to have_gitlab_http_status(:accepted) - expect(response.header['X-GitLab-Trace-Update-Interval']).to eq('30') + expect(response.header['X-GitLab-Trace-Update-Interval']).to eq('60') end end end + context 'when job does not exist anymore' do + it 'returns 403 Forbidden' do + patch_the_trace(job_id: non_existing_record_id) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + context 'when Runner makes a force-patch' do before do force_patch_the_trace @@ -264,7 +272,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do it { expect(response).to have_gitlab_http_status(:forbidden) } end - def patch_the_trace(content = ' appended', request_headers = nil) + def patch_the_trace(content = ' appended', request_headers = nil, job_id: job.id) unless request_headers job.trace.read do |stream| offset = stream.size @@ -274,7 +282,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do end Timecop.travel(job.updated_at + update_interval) do - patch api("/jobs/#{job.id}/trace"), params: content, headers: request_headers + patch api("/jobs/#{job_id}/trace"), params: content, headers: request_headers job.reload end end diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index 10fa15d468f..ac125e81acd 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -14,8 +14,8 @@ RSpec.describe API::CommitStatuses do let(:get_url) { "/projects/#{project.id}/repository/commits/#{sha}/statuses" } context 'ci commit exists' do - let!(:master) { project.ci_pipelines.create(source: :push, sha: commit.id, ref: 'master', protected: false) } - let!(:develop) { project.ci_pipelines.create(source: :push, sha: commit.id, ref: 'develop', protected: false) } + let!(:master) { project.ci_pipelines.create!(source: :push, sha: commit.id, ref: 'master', protected: false) } + let!(:develop) { project.ci_pipelines.create!(source: :push, sha: commit.id, ref: 'develop', protected: false) } context "reporter user" do let(:statuses_id) { json_response.map { |status| status['id'] } } @@ -270,8 +270,8 @@ RSpec.describe API::CommitStatuses do end context 'when a pipeline id is specified' do - let!(:first_pipeline) { project.ci_pipelines.create(source: :push, sha: commit.id, ref: 'master', status: 'created') } - let!(:other_pipeline) { project.ci_pipelines.create(source: :push, sha: commit.id, ref: 'master', status: 'created') } + let!(:first_pipeline) { project.ci_pipelines.create!(source: :push, sha: commit.id, ref: 'master', status: 'created') } + let!(:other_pipeline) { project.ci_pipelines.create!(source: :push, sha: commit.id, ref: 'master', status: 'created') } subject do post api(post_url, developer), params: { diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index de2cfb8fea0..ac3aa808f37 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -1439,6 +1439,22 @@ RSpec.describe API::Commits do it_behaves_like 'ref comments' end end + + context 'multiple notes' do + let!(:note) { create(:diff_note_on_commit, project: project) } + let(:commit) { note.commit } + let(:commit_id) { note.commit_id } + + it 'are returned without N + 1' do + get api(route, current_user) # warm up the cache + + control_count = ActiveRecord::QueryRecorder.new { get api(route, current_user) }.count + + create(:diff_note_on_commit, project: project, author: create(:user)) + + expect { get api(route, current_user) }.not_to exceed_query_limit(control_count) + end + end end context 'when the commit is present on two projects' do @@ -1898,8 +1914,12 @@ RSpec.describe API::Commits do let(:merged_mr) { create(:merge_request, source_project: project, source_branch: 'master', target_branch: 'feature') } let(:commit) { merged_mr.merge_request_diff.commits.last } - it 'returns the correct merge request' do + def perform_request(user) get api("/projects/#{project.id}/repository/commits/#{commit.id}/merge_requests", user) + end + + it 'returns the correct merge request' do + perform_request(user) expect(response).to have_gitlab_http_status(:ok) expect(response).to include_limited_pagination_headers @@ -1910,7 +1930,7 @@ RSpec.describe API::Commits do it 'returns 403 for an unauthorized user' do project.add_guest(user) - get api("/projects/#{project.id}/repository/commits/#{commit.id}/merge_requests", user) + perform_request(user) expect(response).to have_gitlab_http_status(:forbidden) end @@ -1926,11 +1946,21 @@ RSpec.describe API::Commits do let(:non_member) { create(:user) } it 'responds 403 when only members are allowed to read merge requests' do - get api("/projects/#{project.id}/repository/commits/#{commit.id}/merge_requests", non_member) + perform_request(non_member) expect(response).to have_gitlab_http_status(:forbidden) end end + + it 'returns multiple merge requests without N + 1' do + perform_request(user) + + control_count = ActiveRecord::QueryRecorder.new { perform_request(user) }.count + + create(:merge_request, :closed, source_project: project, source_branch: 'master', target_branch: 'feature') + + expect { perform_request(user) }.not_to exceed_query_limit(control_count) + end end describe 'GET /projects/:id/repository/commits/:sha/signature' do diff --git a/spec/requests/api/composer_packages_spec.rb b/spec/requests/api/composer_packages_spec.rb index 30a831d24fd..0ff88cb41a8 100644 --- a/spec/requests/api/composer_packages_spec.rb +++ b/spec/requests/api/composer_packages_spec.rb @@ -434,6 +434,7 @@ RSpec.describe API::ComposerPackages do end it_behaves_like 'process Composer api request', params[:user_role], params[:expected_status], params[:member] + it_behaves_like 'a package tracking event', described_class.name, 'pull_package' end end end diff --git a/spec/requests/api/conan_project_packages_spec.rb b/spec/requests/api/conan_project_packages_spec.rb index fefaf9790b1..da054ed2e96 100644 --- a/spec/requests/api/conan_project_packages_spec.rb +++ b/spec/requests/api/conan_project_packages_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require 'spec_helper' -RSpec.describe API::ConanProjectPackages do +RSpec.describe API::ConanProjectPackages, quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/326194' do include_context 'conan api setup' let(:project_id) { project.id } diff --git a/spec/requests/api/deploy_keys_spec.rb b/spec/requests/api/deploy_keys_spec.rb index 591d994fec9..a01c66a311c 100644 --- a/spec/requests/api/deploy_keys_spec.rb +++ b/spec/requests/api/deploy_keys_spec.rb @@ -3,12 +3,13 @@ require 'spec_helper' RSpec.describe API::DeployKeys do - let(:user) { create(:user) } - let(:maintainer) { create(:user) } - let(:admin) { create(:admin) } - let(:project) { create(:project, creator_id: user.id) } - let(:project2) { create(:project, creator_id: user.id) } - let(:deploy_key) { create(:deploy_key, public: true) } + let_it_be(:user) { create(:user) } + let_it_be(:maintainer) { create(:user) } + let_it_be(:admin) { create(:admin) } + let_it_be(:project) { create(:project, creator_id: user.id) } + let_it_be(:project2) { create(:project, creator_id: user.id) } + + let(:deploy_key) { create(:deploy_key, public: true) } let!(:deploy_keys_project) do create(:deploy_keys_project, project: project, deploy_key: deploy_key) @@ -44,18 +45,30 @@ RSpec.describe API::DeployKeys do end describe 'GET /projects/:id/deploy_keys' do - before do - deploy_key + let(:deploy_key) { create(:deploy_key, public: true, user: admin) } + + def perform_request + get api("/projects/#{project.id}/deploy_keys", admin) end it 'returns array of ssh keys' do - get api("/projects/#{project.id}/deploy_keys", admin) + perform_request expect(response).to have_gitlab_http_status(:ok) expect(response).to include_pagination_headers expect(json_response).to be_an Array expect(json_response.first['title']).to eq(deploy_key.title) end + + it 'returns multiple deploy keys without N + 1' do + perform_request + + control_count = ActiveRecord::QueryRecorder.new { perform_request }.count + + create(:deploy_key, public: true, projects: [project], user: maintainer) + + expect { perform_request }.not_to exceed_query_limit(control_count) + end end describe 'GET /projects/:id/deploy_keys/:key_id' do diff --git a/spec/requests/api/deployments_spec.rb b/spec/requests/api/deployments_spec.rb index 8113de96ac4..c89c59a2151 100644 --- a/spec/requests/api/deployments_spec.rb +++ b/spec/requests/api/deployments_spec.rb @@ -3,22 +3,26 @@ require 'spec_helper' RSpec.describe API::Deployments do - let(:user) { create(:user) } - let(:non_member) { create(:user) } + let_it_be(:user) { create(:user) } + let_it_be(:non_member) { create(:user) } before do project.add_maintainer(user) end describe 'GET /projects/:id/deployments' do - let(:project) { create(:project, :repository) } - let!(:deployment_1) { create(:deployment, :success, project: project, iid: 11, ref: 'master', created_at: Time.now, updated_at: Time.now) } - let!(:deployment_2) { create(:deployment, :success, project: project, iid: 12, ref: 'master', created_at: 1.day.ago, updated_at: 2.hours.ago) } - let!(:deployment_3) { create(:deployment, :success, project: project, iid: 8, ref: 'master', created_at: 2.days.ago, updated_at: 1.hour.ago) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:deployment_1) { create(:deployment, :success, project: project, iid: 11, ref: 'master', created_at: Time.now, updated_at: Time.now) } + let_it_be(:deployment_2) { create(:deployment, :success, project: project, iid: 12, ref: 'master', created_at: 1.day.ago, updated_at: 2.hours.ago) } + let_it_be(:deployment_3) { create(:deployment, :success, project: project, iid: 8, ref: 'master', created_at: 2.days.ago, updated_at: 1.hour.ago) } + + def perform_request(params = {}) + get api("/projects/#{project.id}/deployments", user), params: params + end context 'as member of the project' do it 'returns projects deployments sorted by id asc' do - get api("/projects/#{project.id}/deployments", user) + perform_request expect(response).to have_gitlab_http_status(:ok) expect(response).to include_pagination_headers @@ -32,7 +36,7 @@ RSpec.describe API::Deployments do context 'with updated_at filters specified' do it 'returns projects deployments with last update in specified datetime range' do - get api("/projects/#{project.id}/deployments", user), params: { updated_before: 30.minutes.ago, updated_after: 90.minutes.ago } + perform_request({ updated_before: 30.minutes.ago, updated_after: 90.minutes.ago }) expect(response).to have_gitlab_http_status(:ok) expect(response).to include_pagination_headers @@ -42,10 +46,7 @@ RSpec.describe API::Deployments do context 'with the environment filter specifed' do it 'returns deployments for the environment' do - get( - api("/projects/#{project.id}/deployments", user), - params: { environment: deployment_1.environment.name } - ) + perform_request({ environment: deployment_1.environment.name }) expect(json_response.size).to eq(1) expect(json_response.first['iid']).to eq(deployment_1.iid) @@ -86,6 +87,16 @@ RSpec.describe API::Deployments do end end end + + it 'returns multiple deployments without N + 1' do + perform_request # warm up the cache + + control_count = ActiveRecord::QueryRecorder.new { perform_request }.count + + create(:deployment, :success, project: project, iid: 21, ref: 'master') + + expect { perform_request }.not_to exceed_query_limit(control_count) + end end context 'as non member' do @@ -334,7 +345,7 @@ RSpec.describe API::Deployments do context 'as a maintainer' do it 'returns a 403 when updating a deployment with a build' do - deploy.update(deployable: build) + deploy.update!(deployable: build) put( api("/projects/#{project.id}/deployments/#{deploy.id}", user), @@ -383,7 +394,7 @@ RSpec.describe API::Deployments do end it 'returns a 403 when updating a deployment with a build' do - deploy.update(deployable: build) + deploy.update!(deployable: build) put( api("/projects/#{project.id}/deployments/#{deploy.id}", developer), diff --git a/spec/requests/api/environments_spec.rb b/spec/requests/api/environments_spec.rb index 303e510883d..aa1a4643593 100644 --- a/spec/requests/api/environments_spec.rb +++ b/spec/requests/api/environments_spec.rb @@ -214,7 +214,7 @@ RSpec.describe API::Environments do context 'as a maintainer' do context 'with a stoppable environment' do before do - environment.update(state: :available) + environment.update!(state: :available) post api("/projects/#{project.id}/environments/#{environment.id}/stop", user) end diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index 8cd2f00a718..71a4a1a2784 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -517,6 +517,21 @@ RSpec.describe API::Files do expect(response).to have_gitlab_http_status(:ok) end + context 'when ref is not provided' do + before do + stub_application_setting(default_branch_name: 'main') + end + + it 'returns response :ok', :aggregate_failures do + url = route(file_path) + "/raw" + expect(Gitlab::Workhorse).to receive(:send_git_blob) + + get api(url, current_user), params: {} + + expect(response).to have_gitlab_http_status(:ok) + end + end + it 'returns raw file info for files with dots' do url = route('.gitignore') + "/raw" expect(Gitlab::Workhorse).to receive(:send_git_blob) diff --git a/spec/requests/api/generic_packages_spec.rb b/spec/requests/api/generic_packages_spec.rb index 16d56b6cfbe..a5e40eec919 100644 --- a/spec/requests/api/generic_packages_spec.rb +++ b/spec/requests/api/generic_packages_spec.rb @@ -16,6 +16,7 @@ RSpec.describe API::GenericPackages do let_it_be(:project_deploy_token_ro) { create(:project_deploy_token, deploy_token: deploy_token_ro, project: project) } let_it_be(:deploy_token_wo) { create(:deploy_token, read_package_registry: false, write_package_registry: true) } let_it_be(:project_deploy_token_wo) { create(:project_deploy_token, deploy_token: deploy_token_wo, project: project) } + let(:user) { personal_access_token.user } let(:ci_build) { create(:ci_build, :running, user: user) } @@ -326,6 +327,34 @@ RSpec.describe API::GenericPackages do end end end + + context 'different versions' do + where(:version, :expected_status) do + '1.3.350-20201230123456' | :created + '1.2.3' | :created + '1.2.3g' | :created + '1.2' | :created + '1.2.bananas' | :created + 'v1.2.4-build' | :created + 'd50d836eb3de6177ce6c7a5482f27f9c2c84b672' | :created + '..1.2.3' | :bad_request + '1.2.3-4/../../' | :bad_request + '%2e%2e%2f1.2.3' | :bad_request + end + + with_them do + let(:expected_package_diff_count) { expected_status == :created ? 1 : 0 } + let(:headers) { workhorse_headers.merge(auth_header) } + + subject { upload_file(params, headers, package_version: version) } + + it "returns the #{params[:expected_status]}", :aggregate_failures do + expect { subject }.to change { project.packages.generic.count }.by(expected_package_diff_count) + + expect(response).to have_gitlab_http_status(expected_status) + end + end + end end end @@ -418,8 +447,8 @@ RSpec.describe API::GenericPackages do end end - def upload_file(params, request_headers, send_rewritten_field: true, package_name: 'mypackage', file_name: 'myfile.tar.gz') - url = "/projects/#{project.id}/packages/generic/#{package_name}/0.0.1/#{file_name}" + def upload_file(params, request_headers, send_rewritten_field: true, package_name: 'mypackage', package_version: '0.0.1', file_name: 'myfile.tar.gz') + url = "/projects/#{project.id}/packages/generic/#{package_name}/#{package_version}/#{file_name}" workhorse_finalize( api(url), diff --git a/spec/requests/api/go_proxy_spec.rb b/spec/requests/api/go_proxy_spec.rb index d45e24241b2..e678b6cf1c8 100644 --- a/spec/requests/api/go_proxy_spec.rb +++ b/spec/requests/api/go_proxy_spec.rb @@ -363,7 +363,7 @@ RSpec.describe API::GoProxy do let(:module_name) { base } before do - project.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) end describe 'GET /projects/:id/packages/go/*module_name/@v/list' do @@ -412,7 +412,7 @@ RSpec.describe API::GoProxy do let(:module_name) { base } before do - project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) end describe 'GET /projects/:id/packages/go/*module_name/@v/list' do diff --git a/spec/requests/api/graphql/ci/groups_spec.rb b/spec/requests/api/graphql/ci/groups_spec.rb index 9e81358a152..d1a4395d2c9 100644 --- a/spec/requests/api/graphql/ci/groups_spec.rb +++ b/spec/requests/api/graphql/ci/groups_spec.rb @@ -4,10 +4,15 @@ require 'spec_helper' RSpec.describe 'Query.project.pipeline.stages.groups' do include GraphqlHelpers - let(:project) { create(:project, :repository, :public) } - let(:user) { create(:user) } - let(:pipeline) { create(:ci_pipeline, project: project, user: user) } - let(:group_graphql_data) { graphql_data.dig('project', 'pipeline', 'stages', 'nodes', 0, 'groups', 'nodes') } + let_it_be(:project) { create(:project, :repository, :public) } + let_it_be(:user) { create(:user) } + let_it_be(:pipeline) { create(:ci_pipeline, project: project, user: user) } + let(:group_graphql_data) { graphql_data_at(:project, :pipeline, :stages, :nodes, 0, :groups, :nodes) } + + let_it_be(:ref) { 'master' } + let_it_be(:job_a) { create(:commit_status, pipeline: pipeline, name: 'rspec 0 2', ref: ref) } + let_it_be(:job_b) { create(:ci_build, pipeline: pipeline, name: 'rspec 0 1', ref: ref) } + let_it_be(:job_c) { create(:ci_bridge, pipeline: pipeline, name: 'spinach 0 1', ref: ref) } let(:params) { {} } @@ -38,18 +43,15 @@ RSpec.describe 'Query.project.pipeline.stages.groups' do end before do - create(:commit_status, pipeline: pipeline, name: 'rspec 0 2') - create(:commit_status, pipeline: pipeline, name: 'rspec 0 1') - create(:commit_status, pipeline: pipeline, name: 'spinach 0 1') post_graphql(query, current_user: user) end it_behaves_like 'a working graphql query' it 'returns a array of jobs belonging to a pipeline' do - expect(group_graphql_data.map { |g| g.slice('name', 'size') }).to eq([ - { 'name' => 'rspec', 'size' => 2 }, - { 'name' => 'spinach', 'size' => 1 } - ]) + expect(group_graphql_data).to contain_exactly( + a_hash_including('name' => 'rspec', 'size' => 2), + a_hash_including('name' => 'spinach', 'size' => 1) + ) end end diff --git a/spec/requests/api/graphql/ci/job_spec.rb b/spec/requests/api/graphql/ci/job_spec.rb new file mode 100644 index 00000000000..78f7d3e149b --- /dev/null +++ b/spec/requests/api/graphql/ci/job_spec.rb @@ -0,0 +1,100 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Query.project(fullPath).pipelines.job(id)' do + include GraphqlHelpers + + let_it_be(:user) { create_default(:user) } + let_it_be(:project) { create(:project, :repository, :public) } + let_it_be(:pipeline) { create(:ci_pipeline, project: project) } + + let_it_be(:prepare_stage) { create(:ci_stage_entity, pipeline: pipeline, project: project, name: 'prepare') } + let_it_be(:test_stage) { create(:ci_stage_entity, pipeline: pipeline, project: project, name: 'test') } + + let_it_be(:job_1) { create(:ci_build, pipeline: pipeline, stage: 'prepare', name: 'Job 1') } + let_it_be(:job_2) { create(:ci_build, pipeline: pipeline, stage: 'test', name: 'Job 2') } + let_it_be(:job_3) { create(:ci_build, pipeline: pipeline, stage: 'test', name: 'Job 3') } + + let(:path_to_job) do + [ + [:project, { full_path: project.full_path }], + [:pipelines, { first: 1 }], + [:nodes, nil], + [:job, { id: global_id_of(job_2) }] + ] + end + + let(:query) do + wrap_fields(query_graphql_path(query_path, all_graphql_fields_for(terminal_type))) + end + + describe 'scalar fields' do + let(:path) { [:project, :pipelines, :nodes, 0, :job] } + let(:query_path) { path_to_job } + let(:terminal_type) { 'CiJob' } + + it 'retrieves scalar fields' do + post_graphql(query, current_user: user) + + expect(graphql_data_at(*path)).to match a_hash_including( + 'id' => global_id_of(job_2), + 'name' => job_2.name, + 'allowFailure' => job_2.allow_failure, + 'duration' => job_2.duration, + 'status' => job_2.status.upcase + ) + end + + context 'when fetching by name' do + before do + query_path.last[1] = { name: job_2.name } + end + + it 'retrieves scalar fields' do + post_graphql(query, current_user: user) + + expect(graphql_data_at(*path)).to match a_hash_including( + 'id' => global_id_of(job_2), + 'name' => job_2.name + ) + end + end + end + + describe '.detailedStatus' do + let(:path) { [:project, :pipelines, :nodes, 0, :job, :detailed_status] } + let(:query_path) { path_to_job + [:detailed_status] } + let(:terminal_type) { 'DetailedStatus' } + + it 'retrieves detailed status' do + post_graphql(query, current_user: user) + + expect(graphql_data_at(*path)).to match a_hash_including( + 'text' => 'pending', + 'label' => 'pending', + 'action' => a_hash_including('buttonTitle' => 'Cancel this job', 'icon' => 'cancel') + ) + end + end + + describe '.stage' do + let(:path) { [:project, :pipelines, :nodes, 0, :job, :stage] } + let(:query_path) { path_to_job + [:stage] } + let(:terminal_type) { 'CiStage' } + + it 'returns appropriate data' do + post_graphql(query, current_user: user) + + expect(graphql_data_at(*path)).to match a_hash_including( + 'name' => test_stage.name, + 'jobs' => a_hash_including( + 'nodes' => contain_exactly( + a_hash_including('id' => global_id_of(job_2)), + a_hash_including('id' => global_id_of(job_3)) + ) + ) + ) + end + end +end diff --git a/spec/requests/api/graphql/custom_emoji_query_spec.rb b/spec/requests/api/graphql/custom_emoji_query_spec.rb index d5a423d0eba..874357d9eef 100644 --- a/spec/requests/api/graphql/custom_emoji_query_spec.rb +++ b/spec/requests/api/graphql/custom_emoji_query_spec.rb @@ -16,7 +16,15 @@ RSpec.describe 'getting custom emoji within namespace' do describe "Query CustomEmoji on Group" do def custom_emoji_query(group) - graphql_query_for('group', 'fullPath' => group.full_path) + fields = all_graphql_fields_for('Group') + # TODO: Set required timelogs args elsewhere https://gitlab.com/gitlab-org/gitlab/-/issues/325499 + fields.selection['timelogs(startDate: "2021-03-01" endDate: "2021-03-30")'] = fields.selection.delete('timelogs') + + graphql_query_for( + 'group', + { fullPath: group.full_path }, + fields + ) end it 'returns emojis when authorised' do diff --git a/spec/requests/api/graphql/gitlab_schema_spec.rb b/spec/requests/api/graphql/gitlab_schema_spec.rb index fe1c7c15de2..b41d851439b 100644 --- a/spec/requests/api/graphql/gitlab_schema_spec.rb +++ b/spec/requests/api/graphql/gitlab_schema_spec.rb @@ -125,9 +125,9 @@ RSpec.describe 'GitlabSchema configurations' do subject do queries = [ - { query: graphql_query_for('project', { 'fullPath' => '$fullPath' }, %w(id name description)) }, - { query: graphql_query_for('echo', { 'text' => "$test" }, []), variables: { "test" => "Hello world" } }, - { query: graphql_query_for('project', { 'fullPath' => project.full_path }, "userPermissions { createIssue }") } + { query: graphql_query_for('project', { 'fullPath' => '$fullPath' }, %w(id name description)) }, # Complexity 4 + { query: graphql_query_for('echo', { 'text' => "$test" }, []), variables: { "test" => "Hello world" } }, # Complexity 1 + { query: graphql_query_for('project', { 'fullPath' => project.full_path }, "userPermissions { createIssue }") } # Complexity 3 ] post_multiplex(queries, current_user: current_user) @@ -139,10 +139,9 @@ RSpec.describe 'GitlabSchema configurations' do expect(json_response.last['data']['project']).to be_nil end - it_behaves_like 'imposing query limits' do - it 'fails all queries when only one of the queries is too complex' do - # The `project` query above has a complexity of 5 - allow(GitlabSchema).to receive(:max_query_complexity).and_return 4 + shared_examples 'query is too complex' do |description, max_complexity| + it description, :aggregate_failures do + allow(GitlabSchema).to receive(:max_query_complexity).and_return max_complexity subject @@ -155,11 +154,17 @@ RSpec.describe 'GitlabSchema configurations' do # Expect errors for each query expect(graphql_errors.size).to eq(3) graphql_errors.each do |single_query_errors| - expect_graphql_errors_to_include(/which exceeds max complexity of 4/) + expect_graphql_errors_to_include(/Query has complexity of 8, which exceeds max complexity of #{max_complexity}/) end end end + it_behaves_like 'imposing query limits' do + # The total complexity of the multiplex query above is 8 + it_behaves_like 'query is too complex', 'fails all queries when only one of the queries is too complex', 4 + it_behaves_like 'query is too complex', 'fails when all queries combined are too complex', 7 + end + context 'authentication' do let(:current_user) { project.owner } @@ -191,6 +196,7 @@ RSpec.describe 'GitlabSchema configurations' do complexity: 181, depth: 13, duration_s: 7, + operation_name: 'IntrospectionQuery', used_fields: an_instance_of(Array), used_deprecated_fields: an_instance_of(Array) } diff --git a/spec/requests/api/graphql/group/milestones_spec.rb b/spec/requests/api/graphql/group/milestones_spec.rb index 380eaea17f8..a5b489d72fd 100644 --- a/spec/requests/api/graphql/group/milestones_spec.rb +++ b/spec/requests/api/graphql/group/milestones_spec.rb @@ -9,12 +9,14 @@ RSpec.describe 'Milestones through GroupQuery' do let_it_be(:now) { Time.now } describe 'Get list of milestones from a group' do - let_it_be(:group) { create(:group) } + let_it_be(:parent_group) { create(:group) } + let_it_be(:group) { create(:group, parent: parent_group) } let_it_be(:milestone_1) { create(:milestone, group: group) } let_it_be(:milestone_2) { create(:milestone, group: group, state: :closed, start_date: now, due_date: now + 1.day) } let_it_be(:milestone_3) { create(:milestone, group: group, start_date: now, due_date: now + 2.days) } let_it_be(:milestone_4) { create(:milestone, group: group, state: :closed, start_date: now - 2.days, due_date: now - 1.day) } let_it_be(:milestone_from_other_group) { create(:milestone, group: create(:group)) } + let_it_be(:parent_milestone) { create(:milestone, group: parent_group) } let(:milestone_data) { graphql_data['group']['milestones']['edges'] } @@ -64,14 +66,32 @@ RSpec.describe 'Milestones through GroupQuery' do accessible_group.add_developer(user) end - it 'returns milestones also from subgroups and subprojects visible to user' do - fetch_milestones(user, args) + context 'when including decendants' do + let(:args) { { include_descendants: true } } + + it 'returns milestones also from subgroups and subprojects visible to user' do + fetch_milestones(user, args) + + expect_array_response( + milestone_1.to_global_id.to_s, milestone_2.to_global_id.to_s, + milestone_3.to_global_id.to_s, milestone_4.to_global_id.to_s, + submilestone_1.to_global_id.to_s, submilestone_2.to_global_id.to_s + ) + end + end + + context 'when including ancestors' do + let(:args) { { include_ancestors: true } } - expect_array_response( - milestone_1.to_global_id.to_s, milestone_2.to_global_id.to_s, - milestone_3.to_global_id.to_s, milestone_4.to_global_id.to_s, - submilestone_1.to_global_id.to_s, submilestone_2.to_global_id.to_s - ) + it 'returns milestones from ancestor groups' do + fetch_milestones(user, args) + + expect_array_response( + milestone_1.to_global_id.to_s, milestone_2.to_global_id.to_s, + milestone_3.to_global_id.to_s, milestone_4.to_global_id.to_s, + parent_milestone.to_global_id.to_s + ) + end end end diff --git a/spec/requests/api/graphql/group/timelogs_spec.rb b/spec/requests/api/graphql/group/timelogs_spec.rb new file mode 100644 index 00000000000..6e21a73afa9 --- /dev/null +++ b/spec/requests/api/graphql/group/timelogs_spec.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Timelogs through GroupQuery' do + include GraphqlHelpers + + describe 'Get list of timelogs from a group issues' do + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :public, group: group) } + let_it_be(:milestone) { create(:milestone, group: group) } + let_it_be(:issue) { create(:issue, project: project, milestone: milestone) } + let_it_be(:timelog1) { create(:timelog, issue: issue, user: user, spent_at: '2019-08-13 14:00:00') } + let_it_be(:timelog2) { create(:timelog, issue: issue, user: user, spent_at: '2019-08-10 08:00:00') } + let_it_be(:params) { { startTime: '2019-08-10 12:00:00', endTime: '2019-08-21 12:00:00' } } + + let(:timelogs_data) { graphql_data['group']['timelogs']['nodes'] } + + before do + group.add_developer(user) + end + + context 'when the request is correct' do + before do + post_graphql(query, current_user: user) + end + + it_behaves_like 'a working graphql query' + + it 'returns timelogs successfully' do + expect(response).to have_gitlab_http_status(:ok) + expect(graphql_errors).to be_nil + expect(timelog_array.size).to eq 1 + end + + it 'contains correct data', :aggregate_failures do + username = timelog_array.map { |data| data['user']['username'] } + spent_at = timelog_array.map { |data| data['spentAt'].to_time } + time_spent = timelog_array.map { |data| data['timeSpent'] } + issue_title = timelog_array.map { |data| data['issue']['title'] } + milestone_title = timelog_array.map { |data| data['issue']['milestone']['title'] } + + expect(username).to eq([user.username]) + expect(spent_at.first).to be_like_time(timelog1.spent_at) + expect(time_spent).to eq([timelog1.time_spent]) + expect(issue_title).to eq([issue.title]) + expect(milestone_title).to eq([milestone.title]) + end + + context 'when arguments with no time are present' do + let!(:timelog3) { create(:timelog, issue: issue, user: user, spent_at: '2019-08-10 15:00:00') } + let!(:timelog4) { create(:timelog, issue: issue, user: user, spent_at: '2019-08-21 15:00:00') } + let(:params) { { startDate: '2019-08-10', endDate: '2019-08-21' } } + + it 'sets times as start of day and end of day' do + expect(response).to have_gitlab_http_status(:ok) + expect(timelog_array.size).to eq 2 + end + end + end + + context 'when requests has errors' do + context 'when there are no timelogs present' do + before do + Timelog.delete_all + end + + it 'returns empty result' do + post_graphql(query, current_user: user) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to be_nil + expect(timelogs_data).to be_empty + end + end + + context 'when user has no permission to read group timelogs' do + it 'returns empty result' do + guest = create(:user) + group.add_guest(guest) + post_graphql(query, current_user: guest) + + expect(response).to have_gitlab_http_status(:success) + expect(graphql_errors).to be_nil + expect(timelogs_data).to be_empty + end + end + end + end + + def timelog_array(extract_attribute = nil) + timelogs_data.map do |item| + extract_attribute ? item[extract_attribute] : item + end + end + + def query(timelog_params = params) + timelog_nodes = <<~NODE + nodes { + spentAt + timeSpent + user { + username + } + issue { + title + milestone { + title + } + } + } + NODE + + graphql_query_for( + :group, + { full_path: group.full_path }, + query_graphql_field(:timelogs, timelog_params, timelog_nodes) + ) + end +end diff --git a/spec/requests/api/graphql/group_query_spec.rb b/spec/requests/api/graphql/group_query_spec.rb index 391bae4cfcf..8e4f808f794 100644 --- a/spec/requests/api/graphql/group_query_spec.rb +++ b/spec/requests/api/graphql/group_query_spec.rb @@ -17,7 +17,15 @@ RSpec.describe 'getting group information' do # similar to the API "GET /groups/:id" describe "Query group(fullPath)" do def group_query(group) - graphql_query_for('group', 'fullPath' => group.full_path) + fields = all_graphql_fields_for('Group') + # TODO: Set required timelogs args elsewhere https://gitlab.com/gitlab-org/gitlab/-/issues/325499 + fields.selection['timelogs(startDate: "2021-03-01" endDate: "2021-03-30")'] = fields.selection.delete('timelogs') + + graphql_query_for( + 'group', + { fullPath: group.full_path }, + fields + ) end it_behaves_like 'a working graphql query' do diff --git a/spec/requests/api/graphql/mutations/boards/issues/issue_move_list_spec.rb b/spec/requests/api/graphql/mutations/boards/issues/issue_move_list_spec.rb index e24ab0b07f2..46ec22e7ef8 100644 --- a/spec/requests/api/graphql/mutations/boards/issues/issue_move_list_spec.rb +++ b/spec/requests/api/graphql/mutations/boards/issues/issue_move_list_spec.rb @@ -21,7 +21,8 @@ RSpec.describe 'Reposition and move issue within board lists' do let(:mutation_name) { mutation_class.graphql_name } let(:mutation_result_identifier) { mutation_name.camelize(:lower) } let(:current_user) { user } - let(:params) { { board_id: board.to_global_id.to_s, project_path: project.full_path, iid: issue1.iid.to_s } } + let(:board_id) { global_id_of(board) } + let(:params) { { board_id: board_id, project_path: project.full_path, iid: issue1.iid.to_s } } let(:issue_move_params) do { from_list_id: list1.id, @@ -34,16 +35,44 @@ RSpec.describe 'Reposition and move issue within board lists' do end shared_examples 'returns an error' do - it 'fails with error' do - message = "The resource that you are attempting to access does not exist or you don't have "\ - "permission to perform this action" + let(:message) do + "The resource that you are attempting to access does not exist or you don't have " \ + "permission to perform this action" + end + it 'fails with error' do post_graphql_mutation(mutation(params), current_user: current_user) expect(graphql_errors).to include(a_hash_including('message' => message)) end end + context 'when the board_id is not a board' do + let(:board_id) { global_id_of(project) } + let(:issue_move_params) do + { move_after_id: existing_issue1.id, move_before_id: existing_issue2.id } + end + + it_behaves_like 'returns an error' do + let(:message) { include('does not represent an instance of') } + end + end + + # This test aims to distinguish between the failures to authorize + # :read_issue_board and :update_issue + context 'when the user cannot read the issue board' do + let(:issue_move_params) do + { move_after_id: existing_issue1.id, move_before_id: existing_issue2.id } + end + + before do + allow(Ability).to receive(:allowed?).with(any_args).and_return(true) + allow(Ability).to receive(:allowed?).with(current_user, :read_issue_board, board).and_return(false) + end + + it_behaves_like 'returns an error' + end + context 'when user has access to resources' do context 'when repositioning an issue' do let(:issue_move_params) { { move_after_id: existing_issue1.id, move_before_id: existing_issue2.id } } diff --git a/spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb index 97873b01338..bcede4d37dd 100644 --- a/spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb +++ b/spec/requests/api/graphql/mutations/merge_requests/set_assignees_spec.rb @@ -5,11 +5,12 @@ require 'spec_helper' RSpec.describe 'Setting assignees of a merge request' do include GraphqlHelpers - let(:current_user) { create(:user) } - let(:merge_request) { create(:merge_request) } - let(:project) { merge_request.project } - let(:assignee) { create(:user) } - let(:assignee2) { create(:user) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:current_user) { create(:user, developer_projects: [project]) } + let_it_be(:assignee) { create(:user) } + let_it_be(:assignee2) { create(:user) } + let_it_be_with_reload(:merge_request) { create(:merge_request, source_project: project) } + let(:input) { { assignee_usernames: [assignee.username] } } let(:expected_result) do [{ 'username' => assignee.username }] @@ -44,10 +45,19 @@ RSpec.describe 'Setting assignees of a merge request' do mutation_response['mergeRequest']['assignees']['nodes'] end + def run_mutation! + recorder = ActiveRecord::QueryRecorder.new do + post_graphql_mutation(mutation, current_user: current_user) + end + + expect(recorder.count).to be <= db_query_limit + end + before do - project.add_developer(current_user) project.add_developer(assignee) project.add_developer(assignee2) + + merge_request.update!(assignees: []) end it 'returns an error if the user is not allowed to update the merge request' do @@ -56,23 +66,29 @@ RSpec.describe 'Setting assignees of a merge request' do expect(graphql_errors).not_to be_empty end - it 'does not allow members without the right permission to add assignees' do - user = create(:user) - project.add_guest(user) + context 'when the current user does not have permission to add assignees' do + let(:current_user) { create(:user) } + let(:db_query_limit) { 27 } - post_graphql_mutation(mutation, current_user: user) + it 'does not change the assignees' do + project.add_guest(current_user) - expect(graphql_errors).not_to be_empty + expect { run_mutation! }.not_to change { merge_request.reset.assignees.pluck(:id) } + + expect(graphql_errors).not_to be_empty + end end context 'with assignees already assigned' do + let(:db_query_limit) { 39 } + before do merge_request.assignees = [assignee2] merge_request.save! end it 'replaces the assignee' do - post_graphql_mutation(mutation, current_user: current_user) + run_mutation! expect(response).to have_gitlab_http_status(:success) expect(mutation_assignee_nodes).to match_array(expected_result) @@ -80,6 +96,7 @@ RSpec.describe 'Setting assignees of a merge request' do end context 'when passing an empty list of assignees' do + let(:db_query_limit) { 31 } let(:input) { { assignee_usernames: [] } } before do @@ -88,7 +105,7 @@ RSpec.describe 'Setting assignees of a merge request' do end it 'removes assignee' do - post_graphql_mutation(mutation, current_user: current_user) + run_mutation! expect(response).to have_gitlab_http_status(:success) expect(mutation_assignee_nodes).to eq([]) @@ -96,7 +113,9 @@ RSpec.describe 'Setting assignees of a merge request' do end context 'when passing append as true' do - let(:input) { { assignee_usernames: [assignee2.username], operation_mode: Types::MutationOperationModeEnum.enum[:append] } } + let(:mode) { Types::MutationOperationModeEnum.enum[:append] } + let(:input) { { assignee_usernames: [assignee2.username], operation_mode: mode } } + let(:db_query_limit) { 20 } before do # In CE, APPEND is a NOOP as you can't have multiple assignees @@ -108,7 +127,7 @@ RSpec.describe 'Setting assignees of a merge request' do end it 'does not replace the assignee in CE' do - post_graphql_mutation(mutation, current_user: current_user) + run_mutation! expect(response).to have_gitlab_http_status(:success) expect(mutation_assignee_nodes).to match_array(expected_result) @@ -116,7 +135,9 @@ RSpec.describe 'Setting assignees of a merge request' do end context 'when passing remove as true' do - let(:input) { { assignee_usernames: [assignee.username], operation_mode: Types::MutationOperationModeEnum.enum[:remove] } } + let(:db_query_limit) { 31 } + let(:mode) { Types::MutationOperationModeEnum.enum[:remove] } + let(:input) { { assignee_usernames: [assignee.username], operation_mode: mode } } let(:expected_result) { [] } before do @@ -125,7 +146,7 @@ RSpec.describe 'Setting assignees of a merge request' do end it 'removes the users in the list, while adding none' do - post_graphql_mutation(mutation, current_user: current_user) + run_mutation! expect(response).to have_gitlab_http_status(:success) expect(mutation_assignee_nodes).to match_array(expected_result) diff --git a/spec/requests/api/graphql/mutations/merge_requests/set_labels_spec.rb b/spec/requests/api/graphql/mutations/merge_requests/set_labels_spec.rb index 34d347c76fd..0d0cc66c52a 100644 --- a/spec/requests/api/graphql/mutations/merge_requests/set_labels_spec.rb +++ b/spec/requests/api/graphql/mutations/merge_requests/set_labels_spec.rb @@ -52,7 +52,7 @@ RSpec.describe 'Setting labels of a merge request' do end it 'sets the merge request labels, removing existing ones' do - merge_request.update(labels: [label2]) + merge_request.update!(labels: [label2]) post_graphql_mutation(mutation, current_user: current_user) diff --git a/spec/requests/api/graphql/mutations/release_asset_links/delete_spec.rb b/spec/requests/api/graphql/mutations/release_asset_links/delete_spec.rb new file mode 100644 index 00000000000..57489c82ec2 --- /dev/null +++ b/spec/requests/api/graphql/mutations/release_asset_links/delete_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Deletes a release asset link' do + include GraphqlHelpers + + let_it_be(:project) { create(:project, :private, :repository) } + let_it_be(:release) { create(:release, project: project) } + let_it_be(:maintainer) { create(:user).tap { |u| project.add_maintainer(u) } } + let_it_be(:release_link) { create(:release_link, release: release) } + + let(:current_user) { maintainer } + let(:mutation_name) { :release_asset_link_delete } + let(:mutation_arguments) { { id: release_link.to_global_id.to_s } } + + let(:mutation) do + graphql_mutation(mutation_name, mutation_arguments, <<~FIELDS) + link { + id + name + url + linkType + directAssetUrl + external + } + errors + FIELDS + end + + let(:delete_link) { post_graphql_mutation(mutation, current_user: current_user) } + let(:mutation_response) { graphql_mutation_response(mutation_name)&.with_indifferent_access } + + it 'deletes the release asset link and returns the deleted link', :aggregate_failures do + delete_link + + expected_response = { + id: release_link.to_global_id.to_s, + name: release_link.name, + url: release_link.url, + linkType: release_link.link_type.upcase, + directAssetUrl: end_with(release_link.filepath), + external: true + }.with_indifferent_access + + expect(mutation_response[:link]).to match(expected_response) + expect(mutation_response[:errors]).to eq([]) + end +end diff --git a/spec/requests/api/graphql/mutations/snippets/create_spec.rb b/spec/requests/api/graphql/mutations/snippets/create_spec.rb index 1c2260070ec..d944c9e9e57 100644 --- a/spec/requests/api/graphql/mutations/snippets/create_spec.rb +++ b/spec/requests/api/graphql/mutations/snippets/create_spec.rb @@ -211,5 +211,9 @@ RSpec.describe 'Creating a Snippet' do end end end + + it_behaves_like 'has spam protection' do + let(:mutation_class) { ::Mutations::Snippets::Create } + end end end diff --git a/spec/requests/api/graphql/mutations/snippets/update_spec.rb b/spec/requests/api/graphql/mutations/snippets/update_spec.rb index 43dc8d8bc44..28ab593526a 100644 --- a/spec/requests/api/graphql/mutations/snippets/update_spec.rb +++ b/spec/requests/api/graphql/mutations/snippets/update_spec.rb @@ -157,6 +157,9 @@ RSpec.describe 'Updating a Snippet' do it_behaves_like 'graphql update actions' it_behaves_like 'when the snippet is not found' it_behaves_like 'snippet edit usage data counters' + it_behaves_like 'has spam protection' do + let(:mutation_class) { ::Mutations::Snippets::Update } + end end describe 'ProjectSnippet' do @@ -201,6 +204,10 @@ RSpec.describe 'Updating a Snippet' do end it_behaves_like 'snippet edit usage data counters' + + it_behaves_like 'has spam protection' do + let(:mutation_class) { ::Mutations::Snippets::Update } + end end it_behaves_like 'when the snippet is not found' diff --git a/spec/requests/api/graphql/packages/package_spec.rb b/spec/requests/api/graphql/packages/package_spec.rb index 654215041cb..a0131c7733e 100644 --- a/spec/requests/api/graphql/packages/package_spec.rb +++ b/spec/requests/api/graphql/packages/package_spec.rb @@ -2,33 +2,47 @@ require 'spec_helper' RSpec.describe 'package details' do - using RSpec::Parameterized::TableSyntax include GraphqlHelpers let_it_be(:project) { create(:project) } - let_it_be(:package) { create(:composer_package, project: project) } + let_it_be(:composer_package) { create(:composer_package, project: project) } let_it_be(:composer_json) { { name: 'name', type: 'type', license: 'license', version: 1 } } let_it_be(:composer_metadatum) do # we are forced to manually create the metadatum, without using the factory to force the sha to be a string # and avoid an error where gitaly can't find the repository - create(:composer_metadatum, package: package, target_sha: 'foo_sha', composer_json: composer_json) + create(:composer_metadatum, package: composer_package, target_sha: 'foo_sha', composer_json: composer_json) end let(:depth) { 3 } - let(:excluded) { %w[metadata apiFuzzingCiConfiguration pipeline] } + let(:excluded) { %w[metadata apiFuzzingCiConfiguration pipeline packageFiles] } + let(:metadata) { query_graphql_fragment('ComposerMetadata') } + let(:package_files) {all_graphql_fields_for('PackageFile')} + let(:package_files_metadata) {query_graphql_fragment('ConanFileMetadata')} let(:query) do graphql_query_for(:package, { id: package_global_id }, <<~FIELDS) - #{all_graphql_fields_for('Package', max_depth: depth, excluded: excluded)} + #{all_graphql_fields_for('PackageDetailsType', max_depth: depth, excluded: excluded)} metadata { - #{query_graphql_fragment('ComposerMetadata')} + #{metadata} + } + packageFiles { + nodes { + #{package_files} + fileMetadata { + #{package_files_metadata} + } + } } FIELDS end let(:user) { project.owner } - let(:package_global_id) { global_id_of(package) } + let(:package_global_id) { global_id_of(composer_package) } let(:package_details) { graphql_data_at(:package) } + let(:metadata_response) { graphql_data_at(:package, :metadata) } + let(:package_files_response) { graphql_data_at(:package, :package_files, :nodes) } + let(:first_file_response) { graphql_data_at(:package, :package_files, :nodes, 0)} + let(:first_file_response_metadata) { graphql_data_at(:package, :package_files, :nodes, 0, :file_metadata)} subject { post_graphql(query, current_user: user) } @@ -40,15 +54,68 @@ RSpec.describe 'package details' do it 'matches the JSON schema' do expect(package_details).to match_schema('graphql/packages/package_details') end + end + + describe 'Packages Metadata' do + before do + subject + end - it 'includes the fields of the correct package' do - expect(package_details).to include( - 'id' => package_global_id, - 'metadata' => { + describe 'Composer' do + it 'has the correct metadata' do + expect(metadata_response).to include( 'targetSha' => 'foo_sha', 'composerJson' => composer_json.transform_keys(&:to_s).transform_values(&:to_s) - } - ) + ) + end + + it 'does not have files' do + expect(package_files_response).to be_empty + end + end + + describe 'Conan' do + let_it_be(:conan_package) { create(:conan_package, project: project) } + + let(:package_global_id) { global_id_of(conan_package) } + let(:metadata) { query_graphql_fragment('ConanMetadata') } + let(:first_file) { conan_package.package_files.find { |f| global_id_of(f) == first_file_response['id'] } } + + it 'has the correct metadata' do + expect(metadata_response).to include( + 'id' => global_id_of(conan_package.conan_metadatum), + 'recipe' => conan_package.conan_metadatum.recipe, + 'packageChannel' => conan_package.conan_metadatum.package_channel, + 'packageUsername' => conan_package.conan_metadatum.package_username, + 'recipePath' => conan_package.conan_metadatum.recipe_path + ) + end + + it 'has the right amount of files' do + expect(package_files_response.length).to be(conan_package.package_files.length) + end + + it 'has the basic package files data' do + expect(first_file_response).to include( + 'id' => global_id_of(first_file), + 'fileName' => first_file.file_name, + 'size' => first_file.size.to_s, + 'downloadPath' => first_file.download_path, + 'fileSha1' => first_file.file_sha1, + 'fileMd5' => first_file.file_md5, + 'fileSha256' => first_file.file_sha256 + ) + end + + it 'has the correct file metadata' do + expect(first_file_response_metadata).to include( + 'id' => global_id_of(first_file.conan_file_metadatum), + 'packageRevision' => first_file.conan_file_metadatum.package_revision, + 'conanPackageReference' => first_file.conan_file_metadatum.conan_package_reference, + 'recipeRevision' => first_file.conan_file_metadatum.recipe_revision, + 'conanFileType' => first_file.conan_file_metadatum.conan_file_type.upcase + ) + end end end @@ -56,7 +123,7 @@ RSpec.describe 'package details' do let(:depth) { 3 } let(:excluded) { %w[metadata project tags pipelines] } # to limit the query complexity - let_it_be(:siblings) { create_list(:composer_package, 2, project: project, name: package.name) } + let_it_be(:siblings) { create_list(:composer_package, 2, project: project, name: composer_package.name) } it 'includes the sibling versions' do subject @@ -73,8 +140,32 @@ RSpec.describe 'package details' do subject expect(graphql_data_at(:package, :versions, :nodes, :version)).to be_present - expect(graphql_data_at(:package, :versions, :nodes, :versions)).not_to be_present + expect(graphql_data_at(:package, :versions, :nodes, :versions, :nodes)).to be_empty end end end + + context 'with a batched query' do + let_it_be(:conan_package) { create(:conan_package, project: project) } + + let(:batch_query) do + <<~QUERY + { + a: package(id: "#{global_id_of(composer_package)}") { name } + b: package(id: "#{global_id_of(conan_package)}") { name } + } + QUERY + end + + let(:a_packages_names) { graphql_data_at(:a, :packages, :nodes, :name) } + + it 'returns an error for the second package and data for the first' do + post_graphql(batch_query, current_user: user) + + expect(graphql_data_at(:a, :name)).to eq(composer_package.name) + + expect_graphql_errors_to_include [/Package details can be requested only for one package at a time/] + expect(graphql_data_at(:b)).to be(nil) + end + end end diff --git a/spec/requests/api/graphql/project/alert_management/alert/assignees_spec.rb b/spec/requests/api/graphql/project/alert_management/alert/assignees_spec.rb index 9ab94f1d749..a59402208ec 100644 --- a/spec/requests/api/graphql/project/alert_management/alert/assignees_spec.rb +++ b/spec/requests/api/graphql/project/alert_management/alert/assignees_spec.rb @@ -34,7 +34,7 @@ RSpec.describe 'getting Alert Management Alert Assignees' do end let(:alerts) { graphql_data.dig('project', 'alertManagementAlerts', 'nodes') } - let(:assignees) { alerts.map { |alert| [alert['iid'], alert['assignees']['nodes']] }.to_h } + let(:assignees) { alerts.to_h { |alert| [alert['iid'], alert['assignees']['nodes']] } } let(:first_assignees) { assignees[first_alert.iid.to_s] } let(:second_assignees) { assignees[second_alert.iid.to_s] } diff --git a/spec/requests/api/graphql/project/alert_management/alert/notes_spec.rb b/spec/requests/api/graphql/project/alert_management/alert/notes_spec.rb index 5d46f370756..72d185144ef 100644 --- a/spec/requests/api/graphql/project/alert_management/alert/notes_spec.rb +++ b/spec/requests/api/graphql/project/alert_management/alert/notes_spec.rb @@ -38,7 +38,7 @@ RSpec.describe 'getting Alert Management Alert Notes' do end let(:alerts_result) { graphql_data.dig('project', 'alertManagementAlerts', 'nodes') } - let(:notes_result) { alerts_result.map { |alert| [alert['iid'], alert['notes']['nodes']] }.to_h } + let(:notes_result) { alerts_result.to_h { |alert| [alert['iid'], alert['notes']['nodes']] } } let(:first_notes_result) { notes_result[first_alert.iid.to_s] } let(:second_notes_result) { notes_result[second_alert.iid.to_s] } diff --git a/spec/requests/api/graphql/project/alert_management/alert/todos_spec.rb b/spec/requests/api/graphql/project/alert_management/alert/todos_spec.rb index 3a9077061ad..ca58079fdfe 100644 --- a/spec/requests/api/graphql/project/alert_management/alert/todos_spec.rb +++ b/spec/requests/api/graphql/project/alert_management/alert/todos_spec.rb @@ -34,7 +34,7 @@ RSpec.describe 'getting Alert Management Alert Assignees' do end let(:gql_alerts) { graphql_data.dig('project', 'alertManagementAlerts', 'nodes') } - let(:gql_todos) { gql_alerts.map { |gql_alert| [gql_alert['iid'], gql_alert['todos']['nodes']] }.to_h } + let(:gql_todos) { gql_alerts.to_h { |gql_alert| [gql_alert['iid'], gql_alert['todos']['nodes']] } } let(:gql_alert_todo) { gql_todos[alert.iid.to_s].first } let(:gql_other_alert_todo) { gql_todos[other_alert.iid.to_s].first } diff --git a/spec/requests/api/graphql/project/alert_management/integrations_spec.rb b/spec/requests/api/graphql/project/alert_management/integrations_spec.rb index b13805a61ce..0e029aee9e8 100644 --- a/spec/requests/api/graphql/project/alert_management/integrations_spec.rb +++ b/spec/requests/api/graphql/project/alert_management/integrations_spec.rb @@ -13,6 +13,8 @@ RSpec.describe 'getting Alert Management Integrations' do let_it_be(:inactive_http_integration) { create(:alert_management_http_integration, :inactive, project: project) } let_it_be(:other_project_http_integration) { create(:alert_management_http_integration) } + let(:params) { {} } + let(:fields) do <<~QUERY nodes { @@ -25,7 +27,7 @@ RSpec.describe 'getting Alert Management Integrations' do graphql_query_for( 'project', { 'fullPath' => project.full_path }, - query_graphql_field('alertManagementIntegrations', {}, fields) + query_graphql_field('alertManagementIntegrations', params, fields) ) end @@ -50,34 +52,78 @@ RSpec.describe 'getting Alert Management Integrations' do post_graphql(query, current_user: current_user) end - let(:http_integration) { integrations.first } - let(:prometheus_integration) { integrations.second } + context 'when no extra params given' do + let(:http_integration) { integrations.first } + let(:prometheus_integration) { integrations.second } - it_behaves_like 'a working graphql query' + it_behaves_like 'a working graphql query' + + it { expect(integrations.size).to eq(2) } + + it 'returns the correct properties of the integrations' do + expect(http_integration).to include( + 'id' => global_id_of(active_http_integration), + 'type' => 'HTTP', + 'name' => active_http_integration.name, + 'active' => active_http_integration.active, + 'token' => active_http_integration.token, + 'url' => active_http_integration.url, + 'apiUrl' => nil + ) - it { expect(integrations.size).to eq(2) } - - it 'returns the correct properties of the integrations' do - expect(http_integration).to include( - 'id' => GitlabSchema.id_from_object(active_http_integration).to_s, - 'type' => 'HTTP', - 'name' => active_http_integration.name, - 'active' => active_http_integration.active, - 'token' => active_http_integration.token, - 'url' => active_http_integration.url, - 'apiUrl' => nil - ) - - expect(prometheus_integration).to include( - 'id' => GitlabSchema.id_from_object(prometheus_service).to_s, - 'type' => 'PROMETHEUS', - 'name' => 'Prometheus', - 'active' => prometheus_service.manual_configuration?, - 'token' => project_alerting_setting.token, - 'url' => "http://localhost/#{project.full_path}/prometheus/alerts/notify.json", - 'apiUrl' => prometheus_service.api_url - ) + expect(prometheus_integration).to include( + 'id' => global_id_of(prometheus_service), + 'type' => 'PROMETHEUS', + 'name' => 'Prometheus', + 'active' => prometheus_service.manual_configuration?, + 'token' => project_alerting_setting.token, + 'url' => "http://localhost/#{project.full_path}/prometheus/alerts/notify.json", + 'apiUrl' => prometheus_service.api_url + ) + end end + + context 'when HTTP Integration ID is given' do + let(:params) { { id: global_id_of(active_http_integration) } } + + it_behaves_like 'a working graphql query' + + it { expect(integrations).to be_one } + + it 'returns the correct properties of the HTTP integration' do + expect(integrations.first).to include( + 'id' => global_id_of(active_http_integration), + 'type' => 'HTTP', + 'name' => active_http_integration.name, + 'active' => active_http_integration.active, + 'token' => active_http_integration.token, + 'url' => active_http_integration.url, + 'apiUrl' => nil + ) + end + end + + context 'when Prometheus Integration ID is given' do + let(:params) { { id: global_id_of(prometheus_service) } } + + it_behaves_like 'a working graphql query' + + it { expect(integrations).to be_one } + + it 'returns the correct properties of the Prometheus Integration' do + expect(integrations.first).to include( + 'id' => global_id_of(prometheus_service), + 'type' => 'PROMETHEUS', + 'name' => 'Prometheus', + 'active' => prometheus_service.manual_configuration?, + 'token' => project_alerting_setting.token, + 'url' => "http://localhost/#{project.full_path}/prometheus/alerts/notify.json", + 'apiUrl' => prometheus_service.api_url + ) + end + end + + it_behaves_like 'GraphQL query with several integrations requested', graphql_query_name: 'alertManagementIntegrations' end end end diff --git a/spec/requests/api/graphql/project/container_repositories_spec.rb b/spec/requests/api/graphql/project/container_repositories_spec.rb index 5ffd48a7bc4..3ad56223b61 100644 --- a/spec/requests/api/graphql/project/container_repositories_spec.rb +++ b/spec/requests/api/graphql/project/container_repositories_spec.rb @@ -16,7 +16,7 @@ RSpec.describe 'getting container repositories in a project' do <<~GQL edges { node { - #{all_graphql_fields_for('container_repositories'.classify, excluded: ['pipeline'])} + #{all_graphql_fields_for('container_repositories'.classify, excluded: %w(pipeline jobs))} } } GQL diff --git a/spec/requests/api/graphql/project/issues_spec.rb b/spec/requests/api/graphql/project/issues_spec.rb index 9c915075c42..dd9d44136e5 100644 --- a/spec/requests/api/graphql/project/issues_spec.rb +++ b/spec/requests/api/graphql/project/issues_spec.rb @@ -5,14 +5,15 @@ require 'spec_helper' RSpec.describe 'getting an issue list for a project' do include GraphqlHelpers - let(:issues_data) { graphql_data['project']['issues']['edges'] } - let_it_be(:project) { create(:project, :repository, :public) } let_it_be(:current_user) { create(:user) } let_it_be(:issue_a, reload: true) { create(:issue, project: project, discussion_locked: true) } let_it_be(:issue_b, reload: true) { create(:issue, :with_alert, project: project) } let_it_be(:issues, reload: true) { [issue_a, issue_b] } + let(:issues_data) { graphql_data['project']['issues']['edges'] } + let(:issue_filter_params) { {} } + let(:fields) do <<~QUERY edges { @@ -27,7 +28,7 @@ RSpec.describe 'getting an issue list for a project' do graphql_query_for( 'project', { 'fullPath' => project.full_path }, - query_graphql_field('issues', {}, fields) + query_graphql_field('issues', issue_filter_params, fields) ) end @@ -50,6 +51,16 @@ RSpec.describe 'getting an issue list for a project' do expect(issues_data[1]['node']['discussionLocked']).to eq(true) end + context 'when both assignee_username filters are provided' do + let(:issue_filter_params) { { assignee_username: current_user.username, assignee_usernames: [current_user.username] } } + + it 'returns a mutually exclusive param error' do + post_graphql(query, current_user: current_user) + + expect_graphql_errors_to_include('only one of [assigneeUsernames, assigneeUsername] arguments is allowed at the same time.') + end + end + context 'when limiting the number of results' do let(:query) do <<~GQL @@ -76,7 +87,7 @@ RSpec.describe 'getting an issue list for a project' do end end - context 'no limit is provided' do + context 'when no limit is provided' do let(:issue_limit) { nil } it 'returns all issues' do @@ -143,13 +154,15 @@ RSpec.describe 'getting an issue list for a project' do let_it_be(:data_path) { [:project, :issues] } def pagination_query(params) - graphql_query_for(:project, { full_path: sort_project.full_path }, + graphql_query_for( + :project, + { full_path: sort_project.full_path }, query_graphql_field(:issues, params, "#{page_info} nodes { iid }") ) end def pagination_results_data(data) - data.map { |issue| issue.dig('iid').to_i } + data.map { |issue| issue['iid'].to_i } end context 'when sorting by due date' do @@ -189,27 +202,38 @@ RSpec.describe 'getting an issue list for a project' do it_behaves_like 'sorted paginated query' do let(:sort_param) { :RELATIVE_POSITION_ASC } let(:first_param) { 2 } - let(:expected_results) { [relative_issue5.iid, relative_issue3.iid, relative_issue1.iid, relative_issue4.iid, relative_issue2.iid] } + let(:expected_results) do + [ + relative_issue5.iid, relative_issue3.iid, relative_issue1.iid, + relative_issue4.iid, relative_issue2.iid + ] + end end end end context 'when sorting by priority' do let_it_be(:sort_project) { create(:project, :public) } - let_it_be(:early_milestone) { create(:milestone, project: sort_project, due_date: 10.days.from_now) } - let_it_be(:late_milestone) { create(:milestone, project: sort_project, due_date: 30.days.from_now) } - let_it_be(:priority_label1) { create(:label, project: sort_project, priority: 1) } - let_it_be(:priority_label2) { create(:label, project: sort_project, priority: 5) } - let_it_be(:priority_issue1) { create(:issue, project: sort_project, labels: [priority_label1], milestone: late_milestone) } - let_it_be(:priority_issue2) { create(:issue, project: sort_project, labels: [priority_label2]) } - let_it_be(:priority_issue3) { create(:issue, project: sort_project, milestone: early_milestone) } - let_it_be(:priority_issue4) { create(:issue, project: sort_project) } + let_it_be(:on_project) { { project: sort_project } } + let_it_be(:early_milestone) { create(:milestone, **on_project, due_date: 10.days.from_now) } + let_it_be(:late_milestone) { create(:milestone, **on_project, due_date: 30.days.from_now) } + let_it_be(:priority_1) { create(:label, **on_project, priority: 1) } + let_it_be(:priority_2) { create(:label, **on_project, priority: 5) } + let_it_be(:priority_issue1) { create(:issue, **on_project, labels: [priority_1], milestone: late_milestone) } + let_it_be(:priority_issue2) { create(:issue, **on_project, labels: [priority_2]) } + let_it_be(:priority_issue3) { create(:issue, **on_project, milestone: early_milestone) } + let_it_be(:priority_issue4) { create(:issue, **on_project) } context 'when ascending' do it_behaves_like 'sorted paginated query' do let(:sort_param) { :PRIORITY_ASC } let(:first_param) { 2 } - let(:expected_results) { [priority_issue3.iid, priority_issue1.iid, priority_issue2.iid, priority_issue4.iid] } + let(:expected_results) do + [ + priority_issue3.iid, priority_issue1.iid, + priority_issue2.iid, priority_issue4.iid + ] + end end end @@ -217,7 +241,9 @@ RSpec.describe 'getting an issue list for a project' do it_behaves_like 'sorted paginated query' do let(:sort_param) { :PRIORITY_DESC } let(:first_param) { 2 } - let(:expected_results) { [priority_issue1.iid, priority_issue3.iid, priority_issue2.iid, priority_issue4.iid] } + let(:expected_results) do + [priority_issue1.iid, priority_issue3.iid, priority_issue2.iid, priority_issue4.iid] + end end end end @@ -275,7 +301,7 @@ RSpec.describe 'getting an issue list for a project' do end end - context 'fetching alert management alert' do + context 'when fetching alert management alert' do let(:fields) do <<~QUERY edges { @@ -297,7 +323,7 @@ RSpec.describe 'getting an issue list for a project' do it 'avoids N+1 queries' do control = ActiveRecord::QueryRecorder.new { post_graphql(query, current_user: current_user) } - create(:alert_management_alert, :with_issue, project: project ) + create(:alert_management_alert, :with_issue, project: project) expect { post_graphql(query, current_user: current_user) }.not_to exceed_query_limit(control) end @@ -312,7 +338,7 @@ RSpec.describe 'getting an issue list for a project' do end end - context 'fetching labels' do + context 'when fetching labels' do let(:fields) do <<~QUERY edges { @@ -362,7 +388,7 @@ RSpec.describe 'getting an issue list for a project' do end end - context 'fetching assignees' do + context 'when fetching assignees' do let(:fields) do <<~QUERY edges { @@ -420,9 +446,10 @@ RSpec.describe 'getting an issue list for a project' do query = graphql_query_for( :project, { full_path: project.full_path }, - query_graphql_field(:issues, search_params, [ + query_graphql_field( + :issues, search_params, query_graphql_field(:nodes, nil, requested_fields) - ]) + ) ) post_graphql(query, current_user: current_user) end @@ -448,5 +475,16 @@ RSpec.describe 'getting an issue list for a project' do include_examples 'N+1 query check' end + + context 'when requesting `timelogs`' do + let(:requested_fields) { 'timelogs { nodes { timeSpent } }' } + + before do + create_list(:issue_timelog, 2, issue: issue_a) + create(:issue_timelog, issue: issue_b) + end + + include_examples 'N+1 query check' + end end end diff --git a/spec/requests/api/graphql/project/merge_request_spec.rb b/spec/requests/api/graphql/project/merge_request_spec.rb index e32899c600e..15551005502 100644 --- a/spec/requests/api/graphql/project/merge_request_spec.rb +++ b/spec/requests/api/graphql/project/merge_request_spec.rb @@ -5,21 +5,25 @@ require 'spec_helper' RSpec.describe 'getting merge request information nested in a project' do include GraphqlHelpers - let(:project) { create(:project, :repository, :public) } - let(:current_user) { create(:user) } - let(:merge_request_graphql_data) { graphql_data['project']['mergeRequest'] } - let!(:merge_request) { create(:merge_request, source_project: project) } - let(:mr_fields) { all_graphql_fields_for('MergeRequest', excluded: ['pipeline']) } + let_it_be(:project) { create(:project, :repository, :public) } + let_it_be(:current_user) { create(:user) } + let_it_be_with_reload(:merge_request) { create(:merge_request, source_project: project) } + + let(:merge_request_graphql_data) { graphql_data_at(:project, :merge_request) } + let(:mr_fields) { all_graphql_fields_for('MergeRequest', max_depth: 1) } let(:query) do graphql_query_for( - 'project', - { 'fullPath' => project.full_path }, - query_graphql_field('mergeRequest', { iid: merge_request.iid.to_s }, mr_fields) + :project, + { full_path: project.full_path }, + query_graphql_field(:merge_request, { iid: merge_request.iid.to_s }, mr_fields) ) end it_behaves_like 'a working graphql query' do + # we exclude Project.pipeline because it needs arguments + let(:mr_fields) { all_graphql_fields_for('MergeRequest', excluded: %w[jobs pipeline]) } + before do post_graphql(query, current_user: current_user) end @@ -38,13 +42,17 @@ RSpec.describe 'getting merge request information nested in a project' do expect(merge_request_graphql_data['webUrl']).to be_present end - it 'includes author' do - post_graphql(query, current_user: current_user) + context 'when selecting author' do + let(:mr_fields) { 'author { username }' } - expect(merge_request_graphql_data['author']['username']).to eq(merge_request.author.username) + it 'includes author' do + post_graphql(query, current_user: current_user) + + expect(merge_request_graphql_data['author']['username']).to eq(merge_request.author.username) + end end - context 'the merge_request has reviewers' do + context 'when the merge_request has reviewers' do let(:mr_fields) do <<~SELECT reviewers { nodes { id username } } @@ -68,63 +76,76 @@ RSpec.describe 'getting merge request information nested in a project' do end end - it 'includes diff stats' do - be_natural = an_instance_of(Integer).and(be >= 0) - - post_graphql(query, current_user: current_user) - - sums = merge_request_graphql_data['diffStats'].reduce([0, 0, 0]) do |(a, d, c), node| - a_, d_ = node.values_at('additions', 'deletions') - [a + a_, d + d_, c + a_ + d_] + describe 'diffStats' do + let(:mr_fields) do + <<~FIELDS + diffStats { #{all_graphql_fields_for('DiffStats')} } + diffStatsSummary { #{all_graphql_fields_for('DiffStatsSummary')} } + FIELDS end - expect(merge_request_graphql_data).to include( - 'diffStats' => all(a_hash_including('path' => String, 'additions' => be_natural, 'deletions' => be_natural)), - 'diffStatsSummary' => a_hash_including( - 'fileCount' => merge_request.diff_stats.count, - 'additions' => be_natural, - 'deletions' => be_natural, - 'changes' => be_natural - ) - ) + it 'includes diff stats' do + be_natural = an_instance_of(Integer).and(be >= 0) - # diff_stats is consistent with summary - expect(merge_request_graphql_data['diffStatsSummary'] - .values_at('additions', 'deletions', 'changes')).to eq(sums) - - # diff_stats_summary is internally consistent - expect(merge_request_graphql_data['diffStatsSummary'] - .values_at('additions', 'deletions').sum) - .to eq(merge_request_graphql_data.dig('diffStatsSummary', 'changes')) - .and be_positive - end + post_graphql(query, current_user: current_user) - context 'requesting a specific diff stat' do - let(:diff_stat) { merge_request.diff_stats.first } + sums = merge_request_graphql_data['diffStats'].reduce([0, 0, 0]) do |(a, d, c), node| + a_, d_ = node.values_at('additions', 'deletions') + [a + a_, d + d_, c + a_ + d_] + end - let(:query) do - graphql_query_for(:project, { full_path: project.full_path }, - query_graphql_field(:merge_request, { iid: merge_request.iid.to_s }, [ - query_graphql_field(:diff_stats, { path: diff_stat.path }, all_graphql_fields_for('DiffStats')) - ]) + expect(merge_request_graphql_data).to include( + 'diffStats' => all(a_hash_including('path' => String, 'additions' => be_natural, 'deletions' => be_natural)), + 'diffStatsSummary' => a_hash_including( + 'fileCount' => merge_request.diff_stats.count, + 'additions' => be_natural, + 'deletions' => be_natural, + 'changes' => be_natural + ) ) + + # diff_stats is consistent with summary + expect(merge_request_graphql_data['diffStatsSummary'] + .values_at('additions', 'deletions', 'changes')).to eq(sums) + + # diff_stats_summary is internally consistent + expect(merge_request_graphql_data['diffStatsSummary'] + .values_at('additions', 'deletions').sum) + .to eq(merge_request_graphql_data.dig('diffStatsSummary', 'changes')) + .and be_positive end - it 'includes only the requested stats' do - post_graphql(query, current_user: current_user) + context 'when requesting a specific diff stat' do + let(:diff_stat) { merge_request.diff_stats.first } - expect(merge_request_graphql_data).to include( - 'diffStats' => contain_exactly( - a_hash_including('path' => diff_stat.path, 'additions' => diff_stat.additions, 'deletions' => diff_stat.deletions) + let(:mr_fields) do + query_graphql_field( + :diff_stats, + { path: diff_stat.path }, + all_graphql_fields_for('DiffStats') ) - ) + end + + it 'includes only the requested stats' do + post_graphql(query, current_user: current_user) + + expect(merge_request_graphql_data).to include( + 'diffStats' => contain_exactly( + a_hash_including( + 'path' => diff_stat.path, + 'additions' => diff_stat.additions, + 'deletions' => diff_stat.deletions + ) + ) + ) + end end end it 'includes correct mergedAt value when merged' do time = 1.week.ago merge_request.mark_as_merged - merge_request.metrics.update_columns(merged_at: time) + merge_request.metrics.update!(merged_at: time) post_graphql(query, current_user: current_user) retrieved = merge_request_graphql_data['mergedAt'] @@ -139,7 +160,11 @@ RSpec.describe 'getting merge request information nested in a project' do expect(retrieved).to be_nil end - context 'permissions on the merge request' do + describe 'permissions on the merge request' do + let(:mr_fields) do + "userPermissions { #{all_graphql_fields_for('MergeRequestPermissions')} }" + end + it 'includes the permissions for the current user on a public project' do expected_permissions = { 'readMergeRequest' => true, @@ -162,8 +187,6 @@ RSpec.describe 'getting merge request information nested in a project' do end context 'when the user does not have access to the merge request' do - let(:project) { create(:project, :public, :repository) } - it 'returns nil' do project.project_feature.update!(merge_requests_access_level: ProjectFeature::PRIVATE) @@ -174,13 +197,23 @@ RSpec.describe 'getting merge request information nested in a project' do end context 'when there are pipelines' do - before do + let_it_be(:pipeline) do create( :ci_pipeline, project: merge_request.source_project, ref: merge_request.source_branch, sha: merge_request.diff_head_sha ) + end + + let(:mr_fields) do + <<~FIELDS + headPipeline { id } + pipelines { nodes { id } } + FIELDS + end + + before do merge_request.update_head_pipeline end @@ -193,20 +226,12 @@ RSpec.describe 'getting merge request information nested in a project' do it 'has pipeline connections' do post_graphql(query, current_user: current_user) - expect(merge_request_graphql_data['pipelines']['edges'].size).to eq(1) + expect(merge_request_graphql_data['pipelines']['nodes']).to be_one end end context 'when limiting the number of results' do - let(:merge_requests_graphql_data) { graphql_data['project']['mergeRequests']['edges'] } - - let!(:merge_requests) do - [ - create(:merge_request, source_project: project, source_branch: 'branch-1'), - create(:merge_request, source_project: project, source_branch: 'branch-2'), - create(:merge_request, source_project: project, source_branch: 'branch-3') - ] - end + let(:merge_requests_graphql_data) { graphql_data_at(:project, :merge_requests, :edges) } let(:fields) do <<~QUERY @@ -228,6 +253,10 @@ RSpec.describe 'getting merge request information nested in a project' do end it 'returns the correct number of results' do + create(:merge_request, source_project: project, source_branch: 'branch-1') + create(:merge_request, source_project: project, source_branch: 'branch-2') + create(:merge_request, source_project: project, source_branch: 'branch-3') + post_graphql(query, current_user: current_user) expect(merge_requests_graphql_data.size).to eq 2 @@ -281,4 +310,129 @@ RSpec.describe 'getting merge request information nested in a project' do ) end end + + context 'when requesting information about MR interactions' do + let_it_be(:user) { create(:user) } + + let(:selected_fields) { all_graphql_fields_for('UserMergeRequestInteraction') } + + let(:mr_fields) do + query_nodes( + :reviewers, + query_graphql_field(:merge_request_interaction, nil, selected_fields) + ) + end + + def interaction_data + graphql_data_at(:project, :merge_request, :reviewers, :nodes, :merge_request_interaction) + end + + context 'when the user does not have interactions' do + it 'returns null data' do + post_graphql(query) + + expect(interaction_data).to be_empty + end + end + + context 'when the user is a reviewer, but has not reviewed' do + before do + project.add_guest(user) + merge_request.merge_request_reviewers.create!(reviewer: user) + end + + it 'returns falsey values' do + post_graphql(query) + + expect(interaction_data).to contain_exactly a_hash_including( + 'canMerge' => false, + 'canUpdate' => false, + 'reviewState' => 'UNREVIEWED', + 'reviewed' => false, + 'approved' => false + ) + end + end + + context 'when the user has interacted' do + before do + project.add_maintainer(user) + merge_request.merge_request_reviewers.create!(reviewer: user, state: 'reviewed') + merge_request.approved_by_users << user + end + + it 'returns appropriate data' do + post_graphql(query) + enum = ::Types::MergeRequestReviewStateEnum.values['REVIEWED'] + + expect(interaction_data).to contain_exactly a_hash_including( + 'canMerge' => true, + 'canUpdate' => true, + 'reviewState' => enum.graphql_name, + 'reviewed' => true, + 'approved' => true + ) + end + end + + describe 'scalability' do + let_it_be(:other_users) { create_list(:user, 3) } + + let(:unreviewed) do + { 'reviewState' => 'UNREVIEWED' } + end + + let(:reviewed) do + { 'reviewState' => 'REVIEWED' } + end + + shared_examples 'scalable query for interaction fields' do + before do + ([user] + other_users).each { project.add_guest(_1) } + end + + it 'does not suffer from N+1' do + merge_request.merge_request_reviewers.create!(reviewer: user, state: 'reviewed') + + baseline = ActiveRecord::QueryRecorder.new do + post_graphql(query) + end + + expect(interaction_data).to contain_exactly(include(reviewed)) + + other_users.each do |user| + merge_request.merge_request_reviewers.create!(reviewer: user) + end + + expect { post_graphql(query) }.not_to exceed_query_limit(baseline) + + expect(interaction_data).to contain_exactly( + include(unreviewed), + include(unreviewed), + include(unreviewed), + include(reviewed) + ) + end + end + + context 'when selecting only known scalable fields' do + let(:not_scalable) { %w[canUpdate canMerge] } + let(:selected_fields) do + all_graphql_fields_for('UserMergeRequestInteraction', excluded: not_scalable) + end + + it_behaves_like 'scalable query for interaction fields' + end + + context 'when selecting all fields' do + before do + pending "See: https://gitlab.com/gitlab-org/gitlab/-/issues/322549" + end + + let(:selected_fields) { all_graphql_fields_for('UserMergeRequestInteraction') } + + it_behaves_like 'scalable query for interaction fields' + end + end + end end diff --git a/spec/requests/api/graphql/project/merge_requests_spec.rb b/spec/requests/api/graphql/project/merge_requests_spec.rb index d97a0ed9399..7fc1ef05fa7 100644 --- a/spec/requests/api/graphql/project/merge_requests_spec.rb +++ b/spec/requests/api/graphql/project/merge_requests_spec.rb @@ -47,10 +47,10 @@ RSpec.describe 'getting merge request listings nested in a project' do end before do - # We cannot call the whitelist here, since the transaction does not + # We cannot disable SQL query limiting here, since the transaction does not # begin until we enter the controller. headers = { - 'X-GITLAB-QUERY-WHITELIST-ISSUE' => 'https://gitlab.com/gitlab-org/gitlab/-/issues/322979' + 'X-GITLAB-DISABLE-SQL-QUERY-LIMIT' => 'https://gitlab.com/gitlab-org/gitlab/-/issues/322979' } post_graphql(query, current_user: current_user, headers: headers) @@ -299,6 +299,7 @@ RSpec.describe 'getting merge request listings nested in a project' do reviewers { nodes { username } } participants { nodes { username } } headPipeline { status } + timelogs { nodes { timeSpent } } SELECT end @@ -307,7 +308,7 @@ RSpec.describe 'getting merge request listings nested in a project' do query($first: Int) { project(fullPath: "#{project.full_path}") { mergeRequests(first: $first) { - nodes { #{mr_fields} } + nodes { iid #{mr_fields} } } } } @@ -324,6 +325,7 @@ RSpec.describe 'getting merge request listings nested in a project' do mr.assignees << current_user mr.reviewers << create(:user) mr.reviewers << current_user + mr.timelogs << create(:merge_request_timelog, merge_request: mr) end end @@ -345,7 +347,7 @@ RSpec.describe 'getting merge request listings nested in a project' do end def user_collection - { 'nodes' => all(match(a_hash_including('username' => be_present))) } + { 'nodes' => be_present.and(all(match(a_hash_including('username' => be_present)))) } end it 'returns appropriate results' do @@ -358,7 +360,8 @@ RSpec.describe 'getting merge request listings nested in a project' do 'assignees' => user_collection, 'reviewers' => user_collection, 'participants' => user_collection, - 'headPipeline' => { 'status' => be_present } + 'headPipeline' => { 'status' => be_present }, + 'timelogs' => { 'nodes' => be_one } ))) end diff --git a/spec/requests/api/graphql/project/pipeline_spec.rb b/spec/requests/api/graphql/project/pipeline_spec.rb index cc028ff2ff9..0a5bcc7a965 100644 --- a/spec/requests/api/graphql/project/pipeline_spec.rb +++ b/spec/requests/api/graphql/project/pipeline_spec.rb @@ -5,24 +5,28 @@ require 'spec_helper' RSpec.describe 'getting pipeline information nested in a project' do include GraphqlHelpers - let!(:project) { create(:project, :repository, :public) } - let!(:pipeline) { create(:ci_pipeline, project: project) } - let!(:current_user) { create(:user) } - let(:pipeline_graphql_data) { graphql_data['project']['pipeline'] } - - let!(:query) do - %( - query { - project(fullPath: "#{project.full_path}") { - pipeline(iid: "#{pipeline.iid}") { - configSource - } - } - } + let_it_be(:project) { create(:project, :repository, :public) } + let_it_be(:pipeline) { create(:ci_pipeline, project: project) } + let_it_be(:current_user) { create(:user) } + let_it_be(:build_job) { create(:ci_build, :trace_with_sections, name: 'build-a', pipeline: pipeline) } + let_it_be(:failed_build) { create(:ci_build, :failed, name: 'failed-build', pipeline: pipeline) } + let_it_be(:bridge) { create(:ci_bridge, name: 'ci-bridge-example', pipeline: pipeline) } + + let(:path) { %i[project pipeline] } + let(:pipeline_graphql_data) { graphql_data_at(*path) } + let(:depth) { 3 } + let(:excluded) { %w[job project] } # Project is very expensive, due to the number of fields + let(:fields) { all_graphql_fields_for('Pipeline', excluded: excluded, max_depth: depth) } + + let(:query) do + graphql_query_for( + :project, + { full_path: project.full_path }, + query_graphql_field(:pipeline, { iid: pipeline.iid.to_s }, fields) ) end - it_behaves_like 'a working graphql query' do + it_behaves_like 'a working graphql query', :use_clean_rails_memory_store_caching, :request_store do before do post_graphql(query, current_user: current_user) end @@ -37,14 +41,18 @@ RSpec.describe 'getting pipeline information nested in a project' do it 'contains configSource' do post_graphql(query, current_user: current_user) - expect(pipeline_graphql_data.dig('configSource')).to eq('UNKNOWN_SOURCE') + expect(pipeline_graphql_data['configSource']).to eq('UNKNOWN_SOURCE') end - context 'batching' do - let!(:pipeline2) { create(:ci_pipeline, project: project, user: current_user, builds: [create(:ci_build, :success)]) } - let!(:pipeline3) { create(:ci_pipeline, project: project, user: current_user, builds: [create(:ci_build, :success)]) } + context 'when batching' do + let!(:pipeline2) { successful_pipeline } + let!(:pipeline3) { successful_pipeline } let!(:query) { build_query_to_find_pipeline_shas(pipeline, pipeline2, pipeline3) } + def successful_pipeline + create(:ci_pipeline, project: project, user: current_user, builds: [create(:ci_build, :success)]) + end + it 'executes the finder once' do mock = double(Ci::PipelinesFinder) opts = { iids: [pipeline.iid, pipeline2.iid, pipeline3.iid].map(&:to_s) } @@ -80,4 +88,198 @@ RSpec.describe 'getting pipeline information nested in a project' do graphql_query_for('project', { 'fullPath' => project.full_path }, pipeline_fields) end + + context 'when enough data is requested' do + let(:fields) do + query_graphql_field(:jobs, nil, + query_graphql_field(:nodes, {}, all_graphql_fields_for('CiJob', max_depth: 3))) + end + + it 'contains jobs' do + post_graphql(query, current_user: current_user) + + expect(graphql_data_at(*path, :jobs, :nodes)).to contain_exactly( + a_hash_including( + 'name' => build_job.name, + 'status' => build_job.status.upcase, + 'duration' => build_job.duration + ), + a_hash_including( + 'id' => global_id_of(failed_build), + 'status' => failed_build.status.upcase + ), + a_hash_including( + 'id' => global_id_of(bridge), + 'status' => bridge.status.upcase + ) + ) + end + end + + context 'when requesting only builds with certain statuses' do + let(:variables) do + { + path: project.full_path, + pipelineIID: pipeline.iid.to_s, + status: :FAILED + } + end + + let(:query) do + <<~GQL + query($path: ID!, $pipelineIID: ID!, $status: CiJobStatus!) { + project(fullPath: $path) { + pipeline(iid: $pipelineIID) { + jobs(statuses: [$status]) { + nodes { + #{all_graphql_fields_for('CiJob', max_depth: 1)} + } + } + } + } + } + GQL + end + + it 'can filter build jobs by status' do + post_graphql(query, current_user: current_user, variables: variables) + + expect(graphql_data_at(*path, :jobs, :nodes)) + .to contain_exactly(a_hash_including('id' => global_id_of(failed_build))) + end + end + + context 'when requesting a specific job' do + let(:variables) do + { + path: project.full_path, + pipelineIID: pipeline.iid.to_s + } + end + + let(:build_fields) do + all_graphql_fields_for('CiJob', max_depth: 1) + end + + let(:query) do + <<~GQL + query($path: ID!, $pipelineIID: ID!, $jobName: String, $jobID: JobID) { + project(fullPath: $path) { + pipeline(iid: $pipelineIID) { + job(id: $jobID, name: $jobName) { + #{build_fields} + } + } + } + } + GQL + end + + let(:the_job) do + a_hash_including('name' => build_job.name, 'id' => global_id_of(build_job)) + end + + it 'can request a build by name' do + vars = variables.merge(jobName: build_job.name) + + post_graphql(query, current_user: current_user, variables: vars) + + expect(graphql_data_at(*path, :job)).to match(the_job) + end + + it 'can request a build by ID' do + vars = variables.merge(jobID: global_id_of(build_job)) + + post_graphql(query, current_user: current_user, variables: vars) + + expect(graphql_data_at(*path, :job)).to match(the_job) + end + + context 'when we request nested fields of the build' do + let_it_be(:needy) { create(:ci_build, :dependent, pipeline: pipeline) } + + let(:build_fields) { 'needs { nodes { name } }' } + let(:vars) { variables.merge(jobID: global_id_of(needy)) } + + it 'returns the nested data' do + post_graphql(query, current_user: current_user, variables: vars) + + expect(graphql_data_at(*path, :job, :needs, :nodes)).to contain_exactly( + a_hash_including('name' => needy.needs.first.name) + ) + end + + it 'requires a constant number of queries' do + fst_user = create(:user) + snd_user = create(:user) + path = %i[project pipeline job needs nodes name] + + baseline = ActiveRecord::QueryRecorder.new do + post_graphql(query, current_user: fst_user, variables: vars) + end + + expect(baseline.count).to be > 0 + dep_names = graphql_dig_at(graphql_data(fresh_response_data), *path) + + deps = create_list(:ci_build, 3, :unique_name, pipeline: pipeline) + deps.each { |d| create(:ci_build_need, build: needy, name: d.name) } + + expect do + post_graphql(query, current_user: snd_user, variables: vars) + end.not_to exceed_query_limit(baseline) + + more_names = graphql_dig_at(graphql_data(fresh_response_data), *path) + + expect(more_names).to include(*dep_names) + expect(more_names.count).to be > dep_names.count + end + end + end + + context 'when requesting a specific test suite' do + let_it_be(:pipeline) { create(:ci_pipeline, :with_test_reports, project: project) } + let(:suite_name) { 'test' } + let_it_be(:build_ids) { pipeline.latest_builds.pluck(:id) } + + let(:variables) do + { + path: project.full_path, + pipelineIID: pipeline.iid.to_s + } + end + + let(:query) do + <<~GQL + query($path: ID!, $pipelineIID: ID!, $buildIds: [ID!]!) { + project(fullPath: $path) { + pipeline(iid: $pipelineIID) { + testSuite(buildIds: $buildIds) { + name + } + } + } + } + GQL + end + + it 'can request a test suite by an array of build_ids' do + vars = variables.merge(buildIds: build_ids) + + post_graphql(query, current_user: current_user, variables: vars) + + expect(graphql_data_at(:project, :pipeline, :testSuite, :name)).to eq(suite_name) + end + + context 'when pipeline has no builds that matches the given build_ids' do + let_it_be(:build_ids) { [non_existing_record_id] } + + it 'returns nil' do + vars = variables.merge(buildIds: build_ids) + + post_graphql(query, current_user: current_user, variables: vars) + + expect(graphql_data_at(*path, :test_suite)).to be_nil + end + end + end end diff --git a/spec/requests/api/graphql/project/repository/blobs_spec.rb b/spec/requests/api/graphql/project/repository/blobs_spec.rb new file mode 100644 index 00000000000..12f6fbd793e --- /dev/null +++ b/spec/requests/api/graphql/project/repository/blobs_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true +require 'spec_helper' + +RSpec.describe 'getting blobs in a project repository' do + include GraphqlHelpers + + let(:project) { create(:project, :repository) } + let(:current_user) { project.owner } + let(:paths) { ["CONTRIBUTING.md", "README.md"] } + let(:ref) { project.default_branch } + let(:fields) do + <<~QUERY + blobs(paths:#{paths.inspect}, ref:#{ref.inspect}) { + nodes { + #{all_graphql_fields_for('repository_blob'.classify)} + } + } + QUERY + end + + let(:query) do + graphql_query_for( + 'project', + { 'fullPath' => project.full_path }, + query_graphql_field('repository', {}, fields) + ) + end + + subject(:blobs) { graphql_data_at(:project, :repository, :blobs, :nodes) } + + it 'returns the blob' do + post_graphql(query, current_user: current_user) + + expect(blobs).to match_array(paths.map { |path| a_hash_including('path' => path) }) + end +end diff --git a/spec/requests/api/graphql/user_spec.rb b/spec/requests/api/graphql/user_spec.rb index d2d6b1fca66..a3b2b750bc3 100644 --- a/spec/requests/api/graphql/user_spec.rb +++ b/spec/requests/api/graphql/user_spec.rb @@ -42,7 +42,13 @@ RSpec.describe 'User' do end context 'when username and id parameter are used' do - let_it_be(:query) { graphql_query_for(:user, { id: current_user.to_global_id.to_s, username: current_user.username }, 'id') } + let_it_be(:query) do + graphql_query_for( + :user, + { id: current_user.to_global_id.to_s, username: current_user.username }, + 'id' + ) + end it 'displays an error' do post_graphql(query) diff --git a/spec/requests/api/graphql_spec.rb b/spec/requests/api/graphql_spec.rb index 4eaf57a7d35..3a1bcfc69b8 100644 --- a/spec/requests/api/graphql_spec.rb +++ b/spec/requests/api/graphql_spec.rb @@ -3,16 +3,18 @@ require 'spec_helper' RSpec.describe 'GraphQL' do include GraphqlHelpers + include AfterNextHelpers - let(:query) { graphql_query_for('echo', text: 'Hello world' ) } + let(:query) { graphql_query_for('echo', text: 'Hello world') } - context 'logging' do + describe 'logging' do shared_examples 'logging a graphql query' do let(:expected_params) do { query_string: query, variables: variables.to_s, duration_s: anything, + operation_name: nil, depth: 1, complexity: 1, used_fields: ['Query.echo'], @@ -50,19 +52,25 @@ RSpec.describe 'GraphQL' do context 'when there is an error in the logger' do before do - allow_any_instance_of(Gitlab::Graphql::QueryAnalyzers::LoggerAnalyzer).to receive(:process_variables).and_raise(StandardError.new("oh noes!")) + logger_analyzer = GitlabSchema.query_analyzers.find do |qa| + qa.is_a? Gitlab::Graphql::QueryAnalyzers::LoggerAnalyzer + end + allow(logger_analyzer).to receive(:process_variables) + .and_raise(StandardError.new("oh noes!")) end it 'logs the exception in Sentry and continues with the request' do - expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception).at_least(:once) - expect(Gitlab::GraphqlLogger).to receive(:info) + expect(Gitlab::ErrorTracking) + .to receive(:track_and_raise_for_dev_exception).at_least(:once) + expect(Gitlab::GraphqlLogger) + .to receive(:info) post_graphql(query, variables: {}) end end end - context 'invalid variables' do + context 'with invalid variables' do it 'returns an error' do post_graphql(query, variables: "This is not JSON") @@ -71,7 +79,7 @@ RSpec.describe 'GraphQL' do end end - context 'authentication', :allow_forgery_protection do + describe 'authentication', :allow_forgery_protection do let(:user) { create(:user) } it 'allows access to public data without authentication' do @@ -98,7 +106,7 @@ RSpec.describe 'GraphQL' do expect(graphql_data['echo']).to eq("\"#{user.username}\" says: Hello world") end - context 'token authentication' do + context 'with token authentication' do let(:token) { create(:personal_access_token) } before do @@ -118,7 +126,7 @@ RSpec.describe 'GraphQL' do context 'when the personal access token has no api scope' do it 'does not log the user in' do - token.update(scopes: [:read_user]) + token.update!(scopes: [:read_user]) post_graphql(query, headers: { 'PRIVATE-TOKEN' => token.token }) @@ -135,7 +143,11 @@ RSpec.describe 'GraphQL' do let(:user) { create(:user) } let(:query) do - graphql_query_for('project', { 'fullPath' => project.full_path }, %w(id)) + graphql_query_for( + :project, + { full_path: project.full_path }, + 'id' + ) end before do @@ -196,13 +208,56 @@ RSpec.describe 'GraphQL' do end end + describe 'complexity limits' do + let_it_be(:project) { create(:project, :public) } + let!(:user) { create(:user) } + + let(:query_fields) do + <<~QUERY + id + QUERY + end + + let(:query) do + graphql_query_for( + 'project', + { 'fullPath' => project.full_path }, + query_fields + ) + end + + before do + stub_const('GitlabSchema::DEFAULT_MAX_COMPLEXITY', 1) + end + + context 'unauthenticated user' do + subject { post_graphql(query) } + + it 'raises a complexity error' do + subject + + expect_graphql_errors_to_include(/which exceeds max complexity/) + end + end + + context 'authenticated user' do + subject { post_graphql(query, current_user: user) } + + it 'does not raise an error as it uses the `AUTHENTICATED_COMPLEXITY`' do + subject + + expect(graphql_errors).to be_nil + end + end + end + describe 'keyset pagination' do let_it_be(:project) { create(:project, :public) } let_it_be(:issues) { create_list(:issue, 10, project: project, created_at: Time.now.change(usec: 200)) } let(:page_size) { 6 } - let(:issues_edges) { %w(data project issues edges) } - let(:end_cursor) { %w(data project issues pageInfo endCursor) } + let(:issues_edges) { %w[project issues edges] } + let(:end_cursor) { %w[project issues pageInfo endCursor] } let(:query) do <<~GRAPHQL query project($fullPath: ID!, $first: Int, $after: String) { @@ -216,16 +271,10 @@ RSpec.describe 'GraphQL' do GRAPHQL end - # TODO: Switch this to use `post_graphql` - # This is not performing an actual GraphQL request because the - # variables end up being strings when passed through the `post_graphql` - # helper. - # - # https://gitlab.com/gitlab-org/gitlab/-/issues/222432 def execute_query(after: nil) - GitlabSchema.execute( + post_graphql( query, - context: { current_user: nil }, + current_user: nil, variables: { fullPath: project.full_path, first: page_size, @@ -239,14 +288,16 @@ RSpec.describe 'GraphQL' do expect(Gitlab::Graphql::Pagination::Keyset::QueryBuilder) .to receive(:new).with(anything, anything, hash_including('created_at'), anything).and_call_original - first_page = execute_query + execute_query + first_page = graphql_data edges = first_page.dig(*issues_edges) cursor = first_page.dig(*end_cursor) expect(edges.count).to eq(6) expect(edges.last['node']['iid']).to eq(issues[4].iid.to_s) - second_page = execute_query(after: cursor) + execute_query(after: cursor) + second_page = graphql_data edges = second_page.dig(*issues_edges) expect(edges.count).to eq(4) diff --git a/spec/requests/api/group_import_spec.rb b/spec/requests/api/group_import_spec.rb index bb7436502ed..f632e49bf3a 100644 --- a/spec/requests/api/group_import_spec.rb +++ b/spec/requests/api/group_import_spec.rb @@ -218,12 +218,14 @@ RSpec.describe API::GroupImport do stub_uploads_object_storage(ImportExportUploader, direct_upload: true) end + # rubocop:disable Rails/SaveBang let(:tmp_object) do fog_connection.directories.new(key: 'uploads').files.create( key: "tmp/uploads/#{file_name}", body: file_upload ) end + # rubocop:enable Rails/SaveBang let(:fog_file) { fog_to_uploaded_file(tmp_object) } let(:params) do diff --git a/spec/requests/api/group_milestones_spec.rb b/spec/requests/api/group_milestones_spec.rb index 7ed6e1a295f..e3e0164e5a7 100644 --- a/spec/requests/api/group_milestones_spec.rb +++ b/spec/requests/api/group_milestones_spec.rb @@ -20,7 +20,7 @@ RSpec.describe API::GroupMilestones do let_it_be(:params) { { include_parent_milestones: true } } before_all do - group.update(parent: ancestor_group) + group.update!(parent: ancestor_group) end shared_examples 'listing all milestones' do @@ -64,10 +64,28 @@ RSpec.describe API::GroupMilestones do end end + describe 'GET /groups/:id/milestones/:milestone_id/issues' do + let!(:issue) { create(:issue, project: project, milestone: milestone) } + + def perform_request + get api("/groups/#{group.id}/milestones/#{milestone.id}/issues", user) + end + + it 'returns multiple issues without performing N + 1' do + perform_request + + control_count = ActiveRecord::QueryRecorder.new { perform_request }.count + + create(:issue, project: project, milestone: milestone) + + expect { perform_request }.not_to exceed_query_limit(control_count) + end + end + def setup_for_group - context_group.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + context_group.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC) context_group.add_developer(user) - public_project.update(namespace: context_group) + public_project.update!(namespace: context_group) context_group.reload end end diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb index 0b6bf65ca44..6d5676bbe35 100644 --- a/spec/requests/api/group_variables_spec.rb +++ b/spec/requests/api/group_variables_spec.rb @@ -55,6 +55,7 @@ RSpec.describe API::GroupVariables do expect(json_response['value']).to eq(variable.value) expect(json_response['protected']).to eq(variable.protected?) expect(json_response['variable_type']).to eq(variable.variable_type) + expect(json_response['environment_scope']).to eq(variable.environment_scope) end it 'responds with 404 Not Found if requesting non-existing variable' do @@ -98,6 +99,7 @@ RSpec.describe API::GroupVariables do expect(json_response['protected']).to be_truthy expect(json_response['masked']).to be_truthy expect(json_response['variable_type']).to eq('env_var') + expect(json_response['environment_scope']).to eq('*') end it 'creates variable with optional attributes' do @@ -111,6 +113,7 @@ RSpec.describe API::GroupVariables do expect(json_response['protected']).to be_falsey expect(json_response['masked']).to be_falsey expect(json_response['variable_type']).to eq('file') + expect(json_response['environment_scope']).to eq('*') end it 'does not allow to duplicate variable key' do diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb index 86999c4adaa..6bedd43e5c4 100644 --- a/spec/requests/api/internal/base_spec.rb +++ b/spec/requests/api/internal/base_spec.rb @@ -644,7 +644,7 @@ RSpec.describe API::Internal::Base do context 'with Project' do it_behaves_like 'storing arguments in the application context' do - let(:expected_params) { { user: key.user.username, project: project.full_path } } + let(:expected_params) { { user: key.user.username, project: project.full_path, caller_id: "POST /api/:version/internal/allowed" } } subject { push(key, project) } end @@ -652,7 +652,7 @@ RSpec.describe API::Internal::Base do context 'with PersonalSnippet' do it_behaves_like 'storing arguments in the application context' do - let(:expected_params) { { user: key.user.username } } + let(:expected_params) { { user: key.user.username, caller_id: "POST /api/:version/internal/allowed" } } subject { push(key, personal_snippet) } end @@ -660,7 +660,7 @@ RSpec.describe API::Internal::Base do context 'with ProjectSnippet' do it_behaves_like 'storing arguments in the application context' do - let(:expected_params) { { user: key.user.username, project: project_snippet.project.full_path } } + let(:expected_params) { { user: key.user.username, project: project_snippet.project.full_path, caller_id: "POST /api/:version/internal/allowed" } } subject { push(key, project_snippet) } end @@ -887,7 +887,7 @@ RSpec.describe API::Internal::Base do context 'project does not exist' do context 'git pull' do it 'returns a 200 response with status: false' do - project.destroy + project.destroy! pull(key, project) @@ -1115,7 +1115,7 @@ RSpec.describe API::Internal::Base do end end - context 'feature flag :user_mode_in_session is enabled' do + context 'application setting :admin_mode is enabled' do context 'with an admin user' do let(:user) { create(:admin) } @@ -1147,9 +1147,9 @@ RSpec.describe API::Internal::Base do end end - context 'feature flag :user_mode_in_session is disabled' do + context 'application setting :admin_mode is disabled' do before do - stub_feature_flags(user_mode_in_session: false) + stub_application_setting(admin_mode: false) end context 'with an admin user' do @@ -1413,6 +1413,29 @@ RSpec.describe API::Internal::Base do end end + describe 'GET /internal/geo_proxy' do + subject { get api('/internal/geo_proxy'), params: { secret_token: secret_token } } + + context 'with valid auth' do + it 'returns empty data' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to be_empty + end + end + + context 'with invalid auth' do + let(:secret_token) { 'invalid_token' } + + it 'returns unauthorized' do + subject + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end + def lfs_auth_project(project) post( api("/internal/lfs_authenticate"), diff --git a/spec/requests/api/internal/kubernetes_spec.rb b/spec/requests/api/internal/kubernetes_spec.rb index 2e13016a0a6..47d0c872eb6 100644 --- a/spec/requests/api/internal/kubernetes_spec.rb +++ b/spec/requests/api/internal/kubernetes_spec.rb @@ -38,16 +38,22 @@ RSpec.describe API::Internal::Kubernetes do end shared_examples 'agent authentication' do - it 'returns 403 if Authorization header not sent' do + it 'returns 401 if Authorization header not sent' do send_request - expect(response).to have_gitlab_http_status(:forbidden) + expect(response).to have_gitlab_http_status(:unauthorized) end - it 'returns 403 if Authorization is for non-existent agent' do + it 'returns 401 if Authorization is for non-existent agent' do send_request(headers: { 'Authorization' => 'Bearer NONEXISTENT' }) - expect(response).to have_gitlab_http_status(:forbidden) + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + shared_examples 'agent token tracking' do + it 'tracks token usage' do + expect { response }.to change { agent_token.reload.read_attribute(:last_used_at) } end end @@ -101,6 +107,8 @@ RSpec.describe API::Internal::Kubernetes do let(:agent) { agent_token.agent } let(:project) { agent.project } + shared_examples 'agent token tracking' + it 'returns expected data', :aggregate_failures do send_request(headers: { 'Authorization' => "Bearer #{agent_token.token}" }) @@ -169,6 +177,8 @@ RSpec.describe API::Internal::Kubernetes do context 'an agent is found' do let_it_be(:agent_token) { create(:cluster_agent_token) } + shared_examples 'agent token tracking' + context 'project is public' do let(:project) { create(:project, :public) } diff --git a/spec/requests/api/invitations_spec.rb b/spec/requests/api/invitations_spec.rb index 98a7aa63b16..b0e54055854 100644 --- a/spec/requests/api/invitations_spec.rb +++ b/spec/requests/api/invitations_spec.rb @@ -102,7 +102,8 @@ RSpec.describe API::Invitations do params: { email: stranger.email, access_level: Member::REPORTER } expect(response).to have_gitlab_http_status(:created) - expect(json_response['message'][stranger.email]).to eq("Access level should be greater than or equal to Developer inherited membership from group #{parent.name}") + expect(json_response['message'][stranger.email]) + .to eq("Access level should be greater than or equal to Developer inherited membership from group #{parent.name}") end it 'creates the member if group level is lower' do @@ -153,10 +154,10 @@ RSpec.describe API::Invitations do it "returns a message if member already exists" do post api("/#{source_type.pluralize}/#{source.id}/invitations", maintainer), - params: { email: maintainer.email, access_level: Member::MAINTAINER } + params: { email: developer.email, access_level: Member::MAINTAINER } expect(response).to have_gitlab_http_status(:created) - expect(json_response['message'][maintainer.email]).to eq("Already a member of #{source.name}") + expect(json_response['message'][developer.email]).to eq("User already exists in source") end it 'returns 404 when the email is not valid' do @@ -164,7 +165,7 @@ RSpec.describe API::Invitations do params: { email: '', access_level: Member::MAINTAINER } expect(response).to have_gitlab_http_status(:created) - expect(json_response['message']).to eq('Email cannot be blank') + expect(json_response['message']).to eq('Emails cannot be blank') end it 'returns 404 when the email list is not a valid format' do diff --git a/spec/requests/api/issue_links_spec.rb b/spec/requests/api/issue_links_spec.rb index a4243766111..45583f5c7dc 100644 --- a/spec/requests/api/issue_links_spec.rb +++ b/spec/requests/api/issue_links_spec.rb @@ -12,26 +12,40 @@ RSpec.describe API::IssueLinks do end describe 'GET /links' do + def perform_request(user = nil, params = {}) + get api("/projects/#{project.id}/issues/#{issue.iid}/links", user), params: params + end + context 'when unauthenticated' do it 'returns 401' do - get api("/projects/#{project.id}/issues/#{issue.iid}/links") + perform_request expect(response).to have_gitlab_http_status(:unauthorized) end end context 'when authenticated' do - it 'returns related issues' do - target_issue = create(:issue, project: project) - create(:issue_link, source: issue, target: target_issue) + let_it_be(:issue_link1) { create(:issue_link, source: issue, target: create(:issue, project: project)) } + let_it_be(:issue_link2) { create(:issue_link, source: issue, target: create(:issue, project: project)) } - get api("/projects/#{project.id}/issues/#{issue.iid}/links", user) + it 'returns related issues' do + perform_request(user) expect(response).to have_gitlab_http_status(:ok) expect(json_response).to be_an Array - expect(json_response.length).to eq(1) + expect(json_response.length).to eq(2) expect(response).to match_response_schema('public_api/v4/issue_links') end + + it 'returns multiple links without N + 1' do + perform_request(user) + + control_count = ActiveRecord::QueryRecorder.new { perform_request(user) }.count + + create(:issue_link, source: issue, target: create(:issue, project: project)) + + expect { perform_request(user) }.not_to exceed_query_limit(control_count) + end end end @@ -82,7 +96,7 @@ RSpec.describe API::IssueLinks do params: { target_project_id: unauthorized_project.id, target_issue_iid: target_issue.iid } expect(response).to have_gitlab_http_status(:not_found) - expect(json_response['message']).to eq('No Issue found for given params') + expect(json_response['message']).to eq('No matching issue found. Make sure that you are adding a valid issue URL.') end end diff --git a/spec/requests/api/issues/get_group_issues_spec.rb b/spec/requests/api/issues/get_group_issues_spec.rb index 3870c78deee..cebde747210 100644 --- a/spec/requests/api/issues/get_group_issues_spec.rb +++ b/spec/requests/api/issues/get_group_issues_spec.rb @@ -754,7 +754,7 @@ RSpec.describe API::Issues do let(:parent_group) { create(:group) } before do - group.update(parent_id: parent_group.id) + group.update!(parent_id: parent_group.id) group_closed_issue.reload end diff --git a/spec/requests/api/issues/post_projects_issues_spec.rb b/spec/requests/api/issues/post_projects_issues_spec.rb index 5b3e2363669..7f1db620d4f 100644 --- a/spec/requests/api/issues/post_projects_issues_spec.rb +++ b/spec/requests/api/issues/post_projects_issues_spec.rb @@ -111,7 +111,7 @@ RSpec.describe API::Issues do let(:not_member) { create(:user) } before do - project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE) + project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE) end it 'renders 403' do diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index fe00b654d3b..cff006bed94 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -100,6 +100,18 @@ RSpec.describe API::Jobs do end end + context 'when token is valid but not CI_JOB_TOKEN' do + let(:token) { create(:personal_access_token, user: user) } + + include_context 'with auth headers' do + let(:header) { { 'Private-Token' => token.token } } + end + + it 'returns not found' do + expect(response).to have_gitlab_http_status(:not_found) + end + end + context 'with job token authentication header' do include_context 'with auth headers' do let(:header) { { API::Helpers::Runner::JOB_TOKEN_HEADER => running_job.token } } @@ -215,7 +227,7 @@ RSpec.describe API::Jobs do first_build = create(:ci_build, :trace_artifact, :artifacts, :test_reports, pipeline: pipeline) first_build.runner = create(:ci_runner) first_build.user = create(:user) - first_build.save + first_build.save! control_count = ActiveRecord::QueryRecorder.new { go }.count @@ -223,7 +235,7 @@ RSpec.describe API::Jobs do second_build = create(:ci_build, :trace_artifact, :artifacts, :test_reports, pipeline: second_pipeline) second_build.runner = create(:ci_runner) second_build.user = create(:user) - second_build.save + second_build.save! expect { go }.not_to exceed_query_limit(control_count) end @@ -684,7 +696,7 @@ RSpec.describe API::Jobs do context 'with regular branch' do before do pipeline.reload - pipeline.update(ref: 'master', + pipeline.update!(ref: 'master', sha: project.commit('master').sha) get_for_ref('master') @@ -696,7 +708,7 @@ RSpec.describe API::Jobs do context 'with branch name containing slash' do before do pipeline.reload - pipeline.update(ref: 'improve/awesome', + pipeline.update!(ref: 'improve/awesome', sha: project.commit('improve/awesome').sha) end @@ -732,7 +744,7 @@ RSpec.describe API::Jobs do stub_artifacts_object_storage job.success - project.update(visibility_level: visibility_level, + project.update!(visibility_level: visibility_level, public_builds: public_builds) get_artifact_file(artifact) @@ -826,7 +838,7 @@ RSpec.describe API::Jobs do context 'with branch name containing slash' do before do pipeline.reload - pipeline.update(ref: 'improve/awesome', + pipeline.update!(ref: 'improve/awesome', sha: project.commit('improve/awesome').sha) end diff --git a/spec/requests/api/labels_spec.rb b/spec/requests/api/labels_spec.rb index e3fffd3e3fd..26377c40b73 100644 --- a/spec/requests/api/labels_spec.rb +++ b/spec/requests/api/labels_spec.rb @@ -119,7 +119,7 @@ RSpec.describe API::Labels do expect(label).not_to be_nil - label.priorities.create(project: label.project, priority: 1) + label.priorities.create!(project: label.project, priority: 1) label.save! request_params = { @@ -139,7 +139,7 @@ RSpec.describe API::Labels do expect(label).not_to be_nil label_id = spec_params[:name] || spec_params[:label_id] - label.priorities.create(project: label.project, priority: 1) + label.priorities.create!(project: label.project, priority: 1) label.save! request_params = { @@ -383,7 +383,7 @@ RSpec.describe API::Labels do it 'returns 409 if label already exists in group' do group = create(:group) group_label = create(:group_label, group: group) - project.update(group: group) + project.update!(group: group) post api("/projects/#{project.id}/labels", user), params: { diff --git a/spec/requests/api/lint_spec.rb b/spec/requests/api/lint_spec.rb index cf8cac773f5..f26236e0253 100644 --- a/spec/requests/api/lint_spec.rb +++ b/spec/requests/api/lint_spec.rb @@ -166,7 +166,7 @@ RSpec.describe API::Lint do included_config = YAML.safe_load(included_content, [Symbol]) root_config = YAML.safe_load(yaml_content, [Symbol]) - expected_yaml = included_config.merge(root_config).except(:include).to_yaml + expected_yaml = included_config.merge(root_config).except(:include).deep_stringify_keys.to_yaml expect(response).to have_gitlab_http_status(:ok) expect(json_response).to be_an Hash @@ -246,7 +246,7 @@ RSpec.describe API::Lint do let(:dry_run) { false } let(:included_content) do - { another_test: { stage: 'test', script: 'echo 1' } }.to_yaml + { another_test: { stage: 'test', script: 'echo 1' } }.deep_stringify_keys.to_yaml end before do @@ -299,7 +299,7 @@ RSpec.describe API::Lint do end let(:included_content) do - { another_test: { stage: 'test', script: 'echo 1' } }.to_yaml + { another_test: { stage: 'test', script: 'echo 1' } }.deep_stringify_keys.to_yaml end before do @@ -341,7 +341,7 @@ RSpec.describe API::Lint do context 'with invalid .gitlab-ci.yml content' do let(:yaml_content) do - { image: 'ruby:2.7', services: ['postgres'] }.to_yaml + { image: 'ruby:2.7', services: ['postgres'] }.deep_stringify_keys.to_yaml end before do @@ -385,7 +385,7 @@ RSpec.describe API::Lint do included_config = YAML.safe_load(included_content, [Symbol]) root_config = YAML.safe_load(yaml_content, [Symbol]) - expected_yaml = included_config.merge(root_config).except(:include).to_yaml + expected_yaml = included_config.merge(root_config).except(:include).deep_stringify_keys.to_yaml expect(response).to have_gitlab_http_status(:ok) expect(json_response).to be_an Hash @@ -539,7 +539,7 @@ RSpec.describe API::Lint do context 'with invalid .gitlab-ci.yml content' do let(:yaml_content) do - { image: 'ruby:2.7', services: ['postgres'] }.to_yaml + { image: 'ruby:2.7', services: ['postgres'] }.deep_stringify_keys.to_yaml end context 'when running as dry run' do diff --git a/spec/requests/api/maven_packages_spec.rb b/spec/requests/api/maven_packages_spec.rb index 7f0e4f18e3b..3a015e98fb1 100644 --- a/spec/requests/api/maven_packages_spec.rb +++ b/spec/requests/api/maven_packages_spec.rb @@ -47,7 +47,21 @@ RSpec.describe API::MavenPackages do end end - shared_examples 'processing HEAD requests' do + shared_examples 'rejecting the request for non existing maven path' do |expected_status: :not_found| + before do + if Feature.enabled?(:check_maven_path_first) + expect(::Packages::Maven::PackageFinder).not_to receive(:new) + end + end + + it 'rejects the request' do + subject + + expect(response).to have_gitlab_http_status(expected_status) + end + end + + shared_examples 'processing HEAD requests' do |instance_level: false| subject { head api(url) } before do @@ -92,6 +106,12 @@ RSpec.describe API::MavenPackages do subject end + + context 'with a non existing maven path' do + let(:path) { 'foo/bar/1.2.3' } + + it_behaves_like 'rejecting the request for non existing maven path', expected_status: instance_level ? :forbidden : :not_found + end end end @@ -99,9 +119,8 @@ RSpec.describe API::MavenPackages do context 'successful download' do subject do download_file( - package_file.file_name, - {}, - Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => deploy_token.token + file_name: package_file.file_name, + request_headers: { Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => deploy_token.token } ) end @@ -126,7 +145,7 @@ RSpec.describe API::MavenPackages do shared_examples 'downloads with a job token' do context 'with a running job' do it 'allows download with job token' do - download_file(package_file.file_name, job_token: job.token) + download_file(file_name: package_file.file_name, params: { job_token: job.token }) expect(response).to have_gitlab_http_status(:ok) expect(response.media_type).to eq('application/octet-stream') @@ -139,7 +158,7 @@ RSpec.describe API::MavenPackages do end it 'returns unauthorized error' do - download_file(package_file.file_name, job_token: job.token) + download_file(file_name: package_file.file_name, params: { job_token: job.token }) expect(response).to have_gitlab_http_status(:unauthorized) end @@ -147,133 +166,217 @@ RSpec.describe API::MavenPackages do end describe 'GET /api/v4/packages/maven/*path/:file_name' do - context 'a public project' do - subject { download_file(package_file.file_name) } + shared_examples 'handling all conditions' do + context 'a public project' do + subject { download_file(file_name: package_file.file_name) } - it_behaves_like 'tracking the file download event' + it_behaves_like 'tracking the file download event' - it 'returns the file' do - subject + it 'returns the file' do + subject - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('application/octet-stream') - end + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/octet-stream') + end - it 'returns sha1 of the file' do - download_file(package_file.file_name + '.sha1') + it 'returns sha1 of the file' do + download_file(file_name: package_file.file_name + '.sha1') - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('text/plain') - expect(response.body).to eq(package_file.file_sha1) - end - end + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('text/plain') + expect(response.body).to eq(package_file.file_sha1) + end - context 'internal project' do - before do - project.team.truncate - project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + context 'with a non existing maven path' do + subject { download_file(file_name: package_file.file_name, path: 'foo/bar/1.2.3') } + + it_behaves_like 'rejecting the request for non existing maven path', expected_status: :forbidden + end end - subject { download_file_with_token(package_file.file_name) } + context 'internal project' do + before do + project.team.truncate + project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + end - it_behaves_like 'tracking the file download event' + subject { download_file_with_token(file_name: package_file.file_name) } - it 'returns the file' do - subject + it_behaves_like 'tracking the file download event' - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('application/octet-stream') - end + it 'returns the file' do + subject - it 'denies download when no private token' do - download_file(package_file.file_name) + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/octet-stream') + end - expect(response).to have_gitlab_http_status(:forbidden) - end + it 'denies download when no private token' do + download_file(file_name: package_file.file_name) - it_behaves_like 'downloads with a job token' + expect(response).to have_gitlab_http_status(:forbidden) + end - it_behaves_like 'downloads with a deploy token' - end + it_behaves_like 'downloads with a job token' - context 'private project' do - subject { download_file_with_token(package_file.file_name) } + it_behaves_like 'downloads with a deploy token' - before do - project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + context 'with a non existing maven path' do + subject { download_file_with_token(file_name: package_file.file_name, path: 'foo/bar/1.2.3') } + + it_behaves_like 'rejecting the request for non existing maven path', expected_status: :forbidden + end end - it_behaves_like 'tracking the file download event' + context 'private project' do + subject { download_file_with_token(file_name: package_file.file_name) } - it 'returns the file' do - subject + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('application/octet-stream') - end + it_behaves_like 'tracking the file download event' - it 'denies download when not enough permissions' do - project.add_guest(user) + it 'returns the file' do + subject - subject + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/octet-stream') + end - expect(response).to have_gitlab_http_status(:forbidden) - end + it 'denies download when not enough permissions' do + project.add_guest(user) + + subject - it 'denies download when no private token' do - download_file(package_file.file_name) + expect(response).to have_gitlab_http_status(:forbidden) + end - expect(response).to have_gitlab_http_status(:forbidden) - end + it 'denies download when no private token' do + download_file(file_name: package_file.file_name) - it_behaves_like 'downloads with a job token' + expect(response).to have_gitlab_http_status(:forbidden) + end - it_behaves_like 'downloads with a deploy token' + it_behaves_like 'downloads with a job token' - it 'does not allow download by a unauthorized deploy token with same id as a user with access' do - unauthorized_deploy_token = create(:deploy_token, read_package_registry: true, write_package_registry: true) + it_behaves_like 'downloads with a deploy token' - another_user = create(:user) - project.add_developer(another_user) + it 'does not allow download by a unauthorized deploy token with same id as a user with access' do + unauthorized_deploy_token = create(:deploy_token, read_package_registry: true, write_package_registry: true) - # We force the id of the deploy token and the user to be the same - unauthorized_deploy_token.update!(id: another_user.id) + another_user = create(:user) + project.add_developer(another_user) - download_file( - package_file.file_name, - {}, - Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token - ) + # We force the id of the deploy token and the user to be the same + unauthorized_deploy_token.update!(id: another_user.id) - expect(response).to have_gitlab_http_status(:forbidden) + download_file( + file_name: package_file.file_name, + request_headers: { Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => unauthorized_deploy_token.token } + ) + + expect(response).to have_gitlab_http_status(:forbidden) + end + + context 'with a non existing maven path' do + subject { download_file_with_token(file_name: package_file.file_name, path: 'foo/bar/1.2.3') } + + it_behaves_like 'rejecting the request for non existing maven path', expected_status: :forbidden + end + end + + context 'project name is different from a package name' do + before do + maven_metadatum.update!(path: "wrong_name/#{package.version}") + end + + it 'rejects request' do + download_file(file_name: package_file.file_name) + + expect(response).to have_gitlab_http_status(:forbidden) + end end end - context 'project name is different from a package name' do + context 'with maven_packages_group_level_improvements enabled' do before do - maven_metadatum.update!(path: "wrong_name/#{package.version}") + stub_feature_flags(maven_packages_group_level_improvements: true) end - it 'rejects request' do - download_file(package_file.file_name) + it_behaves_like 'handling all conditions' + end - expect(response).to have_gitlab_http_status(:forbidden) + context 'with maven_packages_group_level_improvements disabled' do + before do + stub_feature_flags(maven_packages_group_level_improvements: false) end + + it_behaves_like 'handling all conditions' end - def download_file(file_name, params = {}, request_headers = headers) - get api("/packages/maven/#{maven_metadatum.path}/#{file_name}"), params: params, headers: request_headers + context 'with check_maven_path_first enabled' do + before do + stub_feature_flags(check_maven_path_first: true) + end + + it_behaves_like 'handling all conditions' + end + + context 'with check_maven_path_first disabled' do + before do + stub_feature_flags(check_maven_path_first: false) + end + + it_behaves_like 'handling all conditions' end - def download_file_with_token(file_name, params = {}, request_headers = headers_with_token) - download_file(file_name, params, request_headers) + def download_file(file_name:, params: {}, request_headers: headers, path: maven_metadatum.path) + get api("/packages/maven/#{path}/#{file_name}"), params: params, headers: request_headers + end + + def download_file_with_token(file_name:, params: {}, request_headers: headers_with_token, path: maven_metadatum.path) + download_file(file_name: file_name, params: params, request_headers: request_headers, path: path) end end describe 'HEAD /api/v4/packages/maven/*path/:file_name' do - let(:url) { "/packages/maven/#{package.maven_metadatum.path}/#{package_file.file_name}" } + let(:path) { package.maven_metadatum.path } + let(:url) { "/packages/maven/#{path}/#{package_file.file_name}" } + + it_behaves_like 'processing HEAD requests', instance_level: true - it_behaves_like 'processing HEAD requests' + context 'with maven_packages_group_level_improvements enabled' do + before do + stub_feature_flags(maven_packages_group_level_improvements: true) + end + + it_behaves_like 'processing HEAD requests', instance_level: true + end + + context 'with maven_packages_group_level_improvements disabled' do + before do + stub_feature_flags(maven_packages_group_level_improvements: false) + end + + it_behaves_like 'processing HEAD requests', instance_level: true + end + + context 'with check_maven_path_first enabled' do + before do + stub_feature_flags(check_maven_path_first: true) + end + + it_behaves_like 'processing HEAD requests', instance_level: true + end + + context 'with check_maven_path_first disabled' do + before do + stub_feature_flags(check_maven_path_first: false) + end + + it_behaves_like 'processing HEAD requests', instance_level: true + end end describe 'GET /api/v4/groups/:id/-/packages/maven/*path/:file_name' do @@ -282,91 +385,299 @@ RSpec.describe API::MavenPackages do group.add_developer(user) end - context 'a public project' do - subject { download_file(package_file.file_name) } + shared_examples 'handling all conditions' do + context 'a public project' do + subject { download_file(file_name: package_file.file_name) } - it_behaves_like 'tracking the file download event' + it_behaves_like 'tracking the file download event' - it 'returns the file' do - subject + it 'returns the file' do + subject - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('application/octet-stream') + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/octet-stream') + end + + it 'returns sha1 of the file' do + download_file(file_name: package_file.file_name + '.sha1') + + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('text/plain') + expect(response.body).to eq(package_file.file_sha1) + end + + context 'with a non existing maven path' do + subject { download_file(file_name: package_file.file_name, path: 'foo/bar/1.2.3') } + + it_behaves_like 'rejecting the request for non existing maven path' + end end - it 'returns sha1 of the file' do - download_file(package_file.file_name + '.sha1') + context 'internal project' do + before do + group.group_member(user).destroy! + project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + end + + subject { download_file_with_token(file_name: package_file.file_name) } - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('text/plain') - expect(response.body).to eq(package_file.file_sha1) + it_behaves_like 'tracking the file download event' + + it 'returns the file' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/octet-stream') + end + + it 'denies download when no private token' do + download_file(file_name: package_file.file_name) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it_behaves_like 'downloads with a job token' + + it_behaves_like 'downloads with a deploy token' + + context 'with a non existing maven path' do + subject { download_file_with_token(file_name: package_file.file_name, path: 'foo/bar/1.2.3') } + + it_behaves_like 'rejecting the request for non existing maven path' + end end - end - context 'internal project' do - before do - group.group_member(user).destroy! - project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL) + context 'private project' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + subject { download_file_with_token(file_name: package_file.file_name) } + + it_behaves_like 'tracking the file download event' + + it 'returns the file' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/octet-stream') + end + + it 'denies download when not enough permissions' do + group.add_guest(user) + + subject + + status = Feature.enabled?(:maven_packages_group_level_improvements, default_enabled: :yaml) ? :not_found : :forbidden + expect(response).to have_gitlab_http_status(status) + end + + it 'denies download when no private token' do + download_file(file_name: package_file.file_name) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it_behaves_like 'downloads with a job token' + + it_behaves_like 'downloads with a deploy token' + + context 'with a non existing maven path' do + subject { download_file_with_token(file_name: package_file.file_name, path: 'foo/bar/1.2.3') } + + it_behaves_like 'rejecting the request for non existing maven path' + end + + context 'with group deploy token' do + subject { download_file_with_token(file_name: package_file.file_name, request_headers: group_deploy_token_headers) } + + it 'returns the file' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/octet-stream') + end + + it 'returns the file with only write_package_registry scope' do + deploy_token_for_group.update!(read_package_registry: false) + + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/octet-stream') + end + + context 'with a non existing maven path' do + subject { download_file_with_token(file_name: package_file.file_name, path: 'foo/bar/1.2.3', request_headers: group_deploy_token_headers) } + + it_behaves_like 'rejecting the request for non existing maven path' + end + end + + context 'with a reporter from a subgroup accessing the root group' do + let_it_be(:root_group) { create(:group, :private) } + let_it_be(:group) { create(:group, :private, parent: root_group) } + + subject { download_file_with_token(file_name: package_file.file_name, request_headers: headers_with_token, group_id: root_group.id) } + + before do + project.update!(namespace: group) + group.add_reporter(user) + end + + it 'returns the file' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/octet-stream') + end + + context 'with a non existing maven path' do + subject { download_file_with_token(file_name: package_file.file_name, path: 'foo/bar/1.2.3', request_headers: headers_with_token, group_id: root_group.id) } + + it_behaves_like 'rejecting the request for non existing maven path' + end + end end - subject { download_file_with_token(package_file.file_name) } + context 'maven metadata file' do + let_it_be(:sub_group1) { create(:group, parent: group) } + let_it_be(:sub_group2) { create(:group, parent: group) } + let_it_be(:project1) { create(:project, :private, group: sub_group1) } + let_it_be(:project2) { create(:project, :private, group: sub_group2) } + let_it_be(:project3) { create(:project, :private, group: sub_group1) } + let_it_be(:package_name) { 'foo' } + let_it_be(:package1) { create(:maven_package, project: project1, name: package_name, version: nil) } + let_it_be(:package_file1) { create(:package_file, :xml, package: package1, file_name: 'maven-metadata.xml') } + let_it_be(:package2) { create(:maven_package, project: project2, name: package_name, version: nil) } + let_it_be(:package_file2) { create(:package_file, :xml, package: package2, file_name: 'maven-metadata.xml') } + let_it_be(:package3) { create(:maven_package, project: project3, name: package_name, version: nil) } + let_it_be(:package_file3) { create(:package_file, :xml, package: package3, file_name: 'maven-metadata.xml') } - it_behaves_like 'tracking the file download event' + let(:maven_metadatum) { package3.maven_metadatum } - it 'returns the file' do - subject + subject { download_file_with_token(file_name: package_file3.file_name) } - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('application/octet-stream') + before do + sub_group1.add_developer(user) + sub_group2.add_developer(user) + # the package with the most recently published file should be returned + create(:package_file, :xml, package: package2) + end + + context 'in multiple versionless packages' do + it 'downloads the file' do + expect(::Packages::PackageFileFinder) + .to receive(:new).with(package2, 'maven-metadata.xml').and_call_original + + subject + end + end + + context 'in multiple snapshot packages' do + before do + version = '1.0.0-SNAPSHOT' + [package1, package2, package3].each do |pkg| + pkg.update!(version: version) + + pkg.maven_metadatum.update!(path: "#{pkg.name}/#{pkg.version}") + end + end + + it 'downloads the file' do + expect(::Packages::PackageFileFinder) + .to receive(:new).with(package3, 'maven-metadata.xml').and_call_original + + subject + end + end + end + end + + context 'with maven_packages_group_level_improvements enabled' do + before do + stub_feature_flags(maven_packages_group_level_improvements: true) end - it 'denies download when no private token' do - download_file(package_file.file_name) + it_behaves_like 'handling all conditions' + end - expect(response).to have_gitlab_http_status(:not_found) + context 'with maven_packages_group_level_improvements disabled' do + before do + stub_feature_flags(maven_packages_group_level_improvements: false) end - it_behaves_like 'downloads with a job token' + it_behaves_like 'handling all conditions' + end + + context 'with check_maven_path_first enabled' do + before do + stub_feature_flags(check_maven_path_first: true) + end - it_behaves_like 'downloads with a deploy token' + it_behaves_like 'handling all conditions' end - context 'private project' do + context 'with check_maven_path_first disabled' do before do - project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + stub_feature_flags(check_maven_path_first: false) end - subject { download_file_with_token(package_file.file_name) } + it_behaves_like 'handling all conditions' + end - it_behaves_like 'tracking the file download event' + def download_file(file_name:, params: {}, request_headers: headers, path: maven_metadatum.path, group_id: group.id) + get api("/groups/#{group_id}/-/packages/maven/#{path}/#{file_name}"), params: params, headers: request_headers + end - it 'returns the file' do - subject + def download_file_with_token(file_name:, params: {}, request_headers: headers_with_token, path: maven_metadatum.path, group_id: group.id) + download_file(file_name: file_name, params: params, request_headers: request_headers, path: path, group_id: group_id) + end + end - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('application/octet-stream') + describe 'HEAD /api/v4/groups/:id/-/packages/maven/*path/:file_name' do + let(:path) { package.maven_metadatum.path } + let(:url) { "/groups/#{group.id}/-/packages/maven/#{path}/#{package_file.file_name}" } + + context 'with maven_packages_group_level_improvements enabled' do + before do + stub_feature_flags(maven_packages_group_level_improvements: true) end - it 'denies download when not enough permissions' do - group.add_guest(user) + it_behaves_like 'processing HEAD requests' + end - subject + context 'with maven_packages_group_level_improvements disabled' do + before do + stub_feature_flags(maven_packages_group_level_improvements: false) + end - expect(response).to have_gitlab_http_status(:forbidden) + it_behaves_like 'processing HEAD requests' + end + + context 'with check_maven_path_first enabled' do + before do + stub_feature_flags(check_maven_path_first: true) end - it 'denies download when no private token' do - download_file(package_file.file_name) + it_behaves_like 'processing HEAD requests' + end - expect(response).to have_gitlab_http_status(:not_found) + context 'with check_maven_path_first disabled' do + before do + stub_feature_flags(check_maven_path_first: false) end - it_behaves_like 'downloads with a job token' + it_behaves_like 'processing HEAD requests' + end + end - it_behaves_like 'downloads with a deploy token' + describe 'GET /api/v4/projects/:id/packages/maven/*path/:file_name' do + shared_examples 'handling all conditions' do + context 'a public project' do + subject { download_file(file_name: package_file.file_name) } - context 'with group deploy token' do - subject { download_file_with_token(package_file.file_name, {}, group_deploy_token_headers) } + it_behaves_like 'tracking the file download event' it 'returns the file' do subject @@ -375,108 +686,145 @@ RSpec.describe API::MavenPackages do expect(response.media_type).to eq('application/octet-stream') end - it 'returns the file with only write_package_registry scope' do - deploy_token_for_group.update!(read_package_registry: false) + it 'returns sha1 of the file' do + download_file(file_name: package_file.file_name + '.sha1') + + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('text/plain') + expect(response.body).to eq(package_file.file_sha1) + end + + context 'with a non existing maven path' do + subject { download_file(file_name: package_file.file_name, path: 'foo/bar/1.2.3') } + + it_behaves_like 'rejecting the request for non existing maven path' + end + end + + context 'private project' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + subject { download_file_with_token(file_name: package_file.file_name) } + + it_behaves_like 'tracking the file download event' + it 'returns the file' do subject expect(response).to have_gitlab_http_status(:ok) expect(response.media_type).to eq('application/octet-stream') end - end - end - def download_file(file_name, params = {}, request_headers = headers) - get api("/groups/#{group.id}/-/packages/maven/#{maven_metadatum.path}/#{file_name}"), params: params, headers: request_headers - end + it 'denies download when not enough permissions' do + project.add_guest(user) - def download_file_with_token(file_name, params = {}, request_headers = headers_with_token) - download_file(file_name, params, request_headers) - end - end - - describe 'HEAD /api/v4/groups/:id/-/packages/maven/*path/:file_name' do - let(:url) { "/groups/#{group.id}/-/packages/maven/#{package.maven_metadatum.path}/#{package_file.file_name}" } + subject - it_behaves_like 'processing HEAD requests' - end + expect(response).to have_gitlab_http_status(:forbidden) + end - describe 'GET /api/v4/projects/:id/packages/maven/*path/:file_name' do - context 'a public project' do - subject { download_file(package_file.file_name) } + it 'denies download when no private token' do + download_file(file_name: package_file.file_name) - it_behaves_like 'tracking the file download event' + expect(response).to have_gitlab_http_status(:not_found) + end - it 'returns the file' do - subject + it_behaves_like 'downloads with a job token' - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('application/octet-stream') - end + it_behaves_like 'downloads with a deploy token' - it 'returns sha1 of the file' do - download_file(package_file.file_name + '.sha1') + context 'with a non existing maven path' do + subject { download_file_with_token(file_name: package_file.file_name, path: 'foo/bar/1.2.3') } - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('text/plain') - expect(response.body).to eq(package_file.file_sha1) + it_behaves_like 'rejecting the request for non existing maven path' + end end end - context 'private project' do + context 'with maven_packages_group_level_improvements enabled' do before do - project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + stub_feature_flags(maven_packages_group_level_improvements: true) end - subject { download_file_with_token(package_file.file_name) } - - it_behaves_like 'tracking the file download event' - - it 'returns the file' do - subject + it_behaves_like 'handling all conditions' + end - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('application/octet-stream') + context 'with maven_packages_group_level_improvements disabled' do + before do + stub_feature_flags(maven_packages_group_level_improvements: false) end - it 'denies download when not enough permissions' do - project.add_guest(user) - - subject + it_behaves_like 'handling all conditions' + end - expect(response).to have_gitlab_http_status(:forbidden) + context 'with check_maven_path_first enabled' do + before do + stub_feature_flags(check_maven_path_first: true) end - it 'denies download when no private token' do - download_file(package_file.file_name) + it_behaves_like 'handling all conditions' + end - expect(response).to have_gitlab_http_status(:not_found) + context 'with check_maven_path_first disabled' do + before do + stub_feature_flags(check_maven_path_first: false) end - it_behaves_like 'downloads with a job token' - - it_behaves_like 'downloads with a deploy token' + it_behaves_like 'handling all conditions' end - def download_file(file_name, params = {}, request_headers = headers) + def download_file(file_name:, params: {}, request_headers: headers, path: maven_metadatum.path) get api("/projects/#{project.id}/packages/maven/" \ - "#{maven_metadatum.path}/#{file_name}"), params: params, headers: request_headers + "#{path}/#{file_name}"), params: params, headers: request_headers end - def download_file_with_token(file_name, params = {}, request_headers = headers_with_token) - download_file(file_name, params, request_headers) + def download_file_with_token(file_name:, params: {}, request_headers: headers_with_token, path: maven_metadatum.path) + download_file(file_name: file_name, params: params, request_headers: request_headers, path: path) end end describe 'HEAD /api/v4/projects/:id/packages/maven/*path/:file_name' do - let(:url) { "/projects/#{project.id}/packages/maven/#{package.maven_metadatum.path}/#{package_file.file_name}" } + let(:path) { package.maven_metadatum.path } + let(:url) { "/projects/#{project.id}/packages/maven/#{path}/#{package_file.file_name}" } + + context 'with maven_packages_group_level_improvements enabled' do + before do + stub_feature_flags(maven_packages_group_level_improvements: true) + end + + it_behaves_like 'processing HEAD requests' + end + + context 'with maven_packages_group_level_improvements disabled' do + before do + stub_feature_flags(maven_packages_group_level_improvements: false) + end + + it_behaves_like 'processing HEAD requests' + end + + context 'with check_maven_path_first enabled' do + before do + stub_feature_flags(check_maven_path_first: true) + end + + it_behaves_like 'processing HEAD requests' + end - it_behaves_like 'processing HEAD requests' + context 'with check_maven_path_first disabled' do + before do + stub_feature_flags(check_maven_path_first: false) + end + + it_behaves_like 'processing HEAD requests' + end end describe 'PUT /api/v4/projects/:id/packages/maven/*path/:file_name/authorize' do it 'rejects a malicious request' do - put api("/projects/#{project.id}/packages/maven/com/example/my-app/#{version}/%2e%2e%2F.ssh%2Fauthorized_keys/authorize"), params: {}, headers: headers_with_token + put api("/projects/#{project.id}/packages/maven/com/example/my-app/#{version}/%2e%2e%2F.ssh%2Fauthorized_keys/authorize"), headers: headers_with_token expect(response).to have_gitlab_http_status(:bad_request) end diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 919c8d29406..d488aee0c10 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -273,7 +273,7 @@ RSpec.describe API::Members do user_ids = [stranger.id, access_requester.id].join(',') allow_next_instance_of(::Members::CreateService) do |service| - expect(service).to receive(:execute).with(source).and_return({ status: :error, message: error_message }) + expect(service).to receive(:execute).and_return({ status: :error, message: error_message }) end expect do @@ -555,6 +555,34 @@ RSpec.describe API::Members do end end + describe 'DELETE /groups/:id/members/:user_id' do + let(:other_user) { create(:user) } + let(:nested_group) { create(:group, parent: group) } + + before do + nested_group.add_developer(developer) + nested_group.add_developer(other_user) + end + + it 'deletes only the member with skip_subresources=true' do + expect do + delete api("/groups/#{group.id}/members/#{developer.id}", maintainer), params: { skip_subresources: true } + + expect(response).to have_gitlab_http_status(:no_content) + end.to change { group.members.count }.by(-1) + .and change { nested_group.members.count }.by(0) + end + + it 'deletes member and its sub memberships with skip_subresources=false' do + expect do + delete api("/groups/#{group.id}/members/#{developer.id}", maintainer), params: { skip_subresources: false } + + expect(response).to have_gitlab_http_status(:no_content) + end.to change { group.members.count }.by(-1) + .and change { nested_group.members.count }.by(-1) + end + end + [false, true].each do |all| it_behaves_like 'GET /:source_type/:id/members/(all)', 'project', all do let(:source) { project } diff --git a/spec/requests/api/merge_request_diffs_spec.rb b/spec/requests/api/merge_request_diffs_spec.rb index 971fb5e991c..caef946273a 100644 --- a/spec/requests/api/merge_request_diffs_spec.rb +++ b/spec/requests/api/merge_request_diffs_spec.rb @@ -75,5 +75,13 @@ RSpec.describe API::MergeRequestDiffs, 'MergeRequestDiffs' do let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/versions/#{merge_request_diff.id}" } end end + + context 'caching merge request diffs', :use_clean_rails_redis_caching do + it 'is performed' do + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/versions/#{merge_request_diff.id}", user) + + expect(Rails.cache.fetch(merge_request_diff.cache_key)).to be_present + end + end end end diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 09177dd1710..37cb8fb7ee5 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -2151,6 +2151,23 @@ RSpec.describe API::MergeRequests do let(:entity) { merge_request } end + context 'when only assignee_ids are provided' do + let(:params) do + { + assignee_ids: [user2.id] + } + end + + it 'sets the assignees' do + put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}", user), params: params + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['assignees']).to contain_exactly( + a_hash_including('name' => user2.name) + ) + end + end + context 'accepts reviewer_ids' do let(:params) do { @@ -2533,7 +2550,7 @@ RSpec.describe API::MergeRequests do it "results in a default squash commit message when not set" do put api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/merge", user), params: { squash: true } - expect(squash_commit.message).to eq(merge_request.default_squash_commit_message) + expect(squash_commit.message.chomp).to eq(merge_request.default_squash_commit_message.chomp) end end diff --git a/spec/requests/api/namespaces_spec.rb b/spec/requests/api/namespaces_spec.rb index 2ac76d469d5..1ed06a40f16 100644 --- a/spec/requests/api/namespaces_spec.rb +++ b/spec/requests/api/namespaces_spec.rb @@ -216,4 +216,77 @@ RSpec.describe API::Namespaces do end end end + + describe 'GET /namespaces/:namespace/exists' do + let!(:namespace1) { create(:group, name: 'Namespace 1', path: 'namespace-1') } + let!(:namespace2) { create(:group, name: 'Namespace 2', path: 'namespace-2') } + let!(:namespace1sub) { create(:group, name: 'Sub Namespace 1', path: 'sub-namespace-1', parent: namespace1) } + let!(:namespace2sub) { create(:group, name: 'Sub Namespace 2', path: 'sub-namespace-2', parent: namespace2) } + + context 'when unauthenticated' do + it 'returns authentication error' do + get api("/namespaces/#{namespace1.path}/exists") + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context 'when authenticated' do + it 'returns JSON indicating the namespace exists and a suggestion' do + get api("/namespaces/#{namespace1.path}/exists", user) + + expected_json = { exists: true, suggests: ["#{namespace1.path}1"] }.to_json + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to eq(expected_json) + end + + it 'returns JSON indicating the namespace does not exist without a suggestion' do + get api("/namespaces/non-existing-namespace/exists", user) + + expected_json = { exists: false, suggests: [] }.to_json + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to eq(expected_json) + end + + it 'checks the existence of a namespace in case-insensitive manner' do + get api("/namespaces/#{namespace1.path.upcase}/exists", user) + + expected_json = { exists: true, suggests: ["#{namespace1.path.upcase}1"] }.to_json + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to eq(expected_json) + end + + it 'checks the existence within the parent namespace only' do + get api("/namespaces/#{namespace1sub.path}/exists", user), params: { parent_id: namespace1.id } + + expected_json = { exists: true, suggests: ["#{namespace1sub.path}1"] }.to_json + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to eq(expected_json) + end + + it 'ignores nested namespaces when checking for top-level namespace' do + get api("/namespaces/#{namespace1sub.path}/exists", user) + + expected_json = { exists: false, suggests: [] }.to_json + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to eq(expected_json) + end + + it 'ignores top-level namespaces when checking with parent_id' do + get api("/namespaces/#{namespace1.path}/exists", user), params: { parent_id: namespace1.id } + + expected_json = { exists: false, suggests: [] }.to_json + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to eq(expected_json) + end + + it 'ignores namespaces of other parent namespaces when checking with parent_id' do + get api("/namespaces/#{namespace2sub.path}/exists", user), params: { parent_id: namespace1.id } + + expected_json = { exists: false, suggests: [] }.to_json + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to eq(expected_json) + end + end + end end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index baab72d106f..d4f8b841c96 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -251,7 +251,7 @@ RSpec.describe API::Notes do expect { subject }.not_to change { Note.where(system: false).count } end - it 'does however create a system note about the change' do + it 'does however create a system note about the change', :sidekiq_inline do expect { subject }.to change { Note.system.count }.by(1) end diff --git a/spec/requests/api/npm_project_packages_spec.rb b/spec/requests/api/npm_project_packages_spec.rb index e64b5ddc374..10271719a15 100644 --- a/spec/requests/api/npm_project_packages_spec.rb +++ b/spec/requests/api/npm_project_packages_spec.rb @@ -41,6 +41,15 @@ RSpec.describe API::NpmProjectPackages do project.add_developer(user) end + shared_examples 'successfully downloads the file' do + it 'returns the file' do + subject + + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/octet-stream') + end + end + shared_examples 'a package file that requires auth' do it 'denies download with no token' do subject @@ -51,35 +60,28 @@ RSpec.describe API::NpmProjectPackages do context 'with access token' do let(:headers) { build_token_auth_header(token.token) } - it 'returns the file' do - subject - - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('application/octet-stream') - end + it_behaves_like 'successfully downloads the file' end context 'with job token' do let(:headers) { build_token_auth_header(job.token) } - it 'returns the file' do - subject - - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('application/octet-stream') - end + it_behaves_like 'successfully downloads the file' end end context 'a public project' do - it 'returns the file with no token needed' do - subject + it_behaves_like 'successfully downloads the file' + it_behaves_like 'a package tracking event', 'API::NpmPackages', 'pull_package' - expect(response).to have_gitlab_http_status(:ok) - expect(response.media_type).to eq('application/octet-stream') - end + context 'with a job token for a different user' do + let_it_be(:other_user) { create(:user) } + let_it_be_with_reload(:other_job) { create(:ci_build, :running, user: other_user) } - it_behaves_like 'a package tracking event', 'API::NpmPackages', 'pull_package' + let(:headers) { build_token_auth_header(other_job.token) } + + it_behaves_like 'successfully downloads the file' + end end context 'private project' do diff --git a/spec/requests/api/nuget_group_packages_spec.rb b/spec/requests/api/nuget_group_packages_spec.rb index f7e81494660..aefbc89dc3b 100644 --- a/spec/requests/api/nuget_group_packages_spec.rb +++ b/spec/requests/api/nuget_group_packages_spec.rb @@ -69,7 +69,7 @@ RSpec.describe API::NugetGroupPackages do let(:take) { 26 } let(:skip) { 0 } let(:include_prereleases) { true } - let(:query_parameters) { { q: search_term, take: take, skip: skip, prerelease: include_prereleases } } + let(:query_parameters) { { q: search_term, take: take, skip: skip, prerelease: include_prereleases }.compact } subject { get api(url), headers: {}} @@ -113,6 +113,45 @@ RSpec.describe API::NugetGroupPackages do end end + context 'with a reporter of subgroup' do + let_it_be(:package_name) { 'Dummy.Package' } + let_it_be(:package) { create(:nuget_package, :with_metadatum, name: package_name, project: project) } + + let(:headers) { basic_auth_header(user.username, personal_access_token.token) } + + subject { get api(url), headers: headers } + + before do + subgroup.add_reporter(user) + project.update_column(:visibility_level, Gitlab::VisibilityLevel.level_value('private')) + subgroup.update_column(:visibility_level, Gitlab::VisibilityLevel.level_value('private')) + group.update_column(:visibility_level, Gitlab::VisibilityLevel.level_value('private')) + end + + describe 'GET /api/v4/groups/:id/-/packages/nuget/metadata/*package_name/index' do + let(:url) { "/groups/#{group.id}/-/packages/nuget/metadata/#{package_name}/index.json" } + + it_behaves_like 'returning response status', :forbidden + end + + describe 'GET /api/v4/groups/:id/-/packages/nuget/metadata/*package_name/*package_version' do + let(:url) { "/groups/#{group.id}/-/packages/nuget/metadata/#{package_name}/#{package.version}.json" } + + it_behaves_like 'returning response status', :forbidden + end + + describe 'GET /api/v4/groups/:id/-/packages/nuget/query' do + let(:search_term) { 'uMmy' } + let(:take) { 26 } + let(:skip) { 0 } + let(:include_prereleases) { false } + let(:query_parameters) { { q: search_term, take: take, skip: skip, prerelease: include_prereleases }.compact } + let(:url) { "/groups/#{group.id}/-/packages/nuget/query?#{query_parameters.to_query}" } + + it_behaves_like 'returning response status', :forbidden + end + end + def update_visibility_to(visibility) project.update!(visibility_level: visibility) subgroup.update!(visibility_level: visibility) diff --git a/spec/requests/api/nuget_project_packages_spec.rb b/spec/requests/api/nuget_project_packages_spec.rb index 0277aa73220..54fe0b985df 100644 --- a/spec/requests/api/nuget_project_packages_spec.rb +++ b/spec/requests/api/nuget_project_packages_spec.rb @@ -188,6 +188,10 @@ RSpec.describe API::NugetProjectPackages do it_behaves_like 'deploy token for package uploads' + it_behaves_like 'job token for package uploads', authorize_endpoint: true do + let_it_be(:job) { create(:ci_build, :running, user: user) } + end + it_behaves_like 'rejects nuget access with unknown target id' it_behaves_like 'rejects nuget access with invalid target id' @@ -251,6 +255,10 @@ RSpec.describe API::NugetProjectPackages do it_behaves_like 'deploy token for package uploads' + it_behaves_like 'job token for package uploads' do + let_it_be(:job) { create(:ci_build, :running, user: user) } + end + it_behaves_like 'rejects nuget access with unknown target id' it_behaves_like 'rejects nuget access with invalid target id' diff --git a/spec/requests/api/project_attributes.yml b/spec/requests/api/project_attributes.yml index 6c9a845b217..f9eb9de94db 100644 --- a/spec/requests/api/project_attributes.yml +++ b/spec/requests/api/project_attributes.yml @@ -140,6 +140,7 @@ project_setting: - squash_option - updated_at - cve_id_request_enabled + - mr_default_target_self build_service_desk_setting: # service_desk_setting unexposed_attributes: diff --git a/spec/requests/api/project_import_spec.rb b/spec/requests/api/project_import_spec.rb index a049d7d7515..f6cdf370e5c 100644 --- a/spec/requests/api/project_import_spec.rb +++ b/spec/requests/api/project_import_spec.rb @@ -235,12 +235,14 @@ RSpec.describe API::ProjectImport do stub_uploads_object_storage(ImportExportUploader, direct_upload: true) end + # rubocop:disable Rails/SaveBang let(:tmp_object) do fog_connection.directories.new(key: 'uploads').files.create( key: "tmp/uploads/#{file_name}", body: fixture_file_upload(file) ) end + # rubocop:enable Rails/SaveBang let(:file_upload) { fog_to_uploaded_file(tmp_object) } @@ -285,7 +287,7 @@ RSpec.describe API::ProjectImport do it 'returns the import status and the error if failed' do project = create(:project, :import_failed) project.add_maintainer(user) - project.import_state.update(last_error: 'error') + project.import_state.update!(last_error: 'error') get api("/projects/#{project.id}/import", user) diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index d2a33e32b30..b0ecb711283 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.shared_examples 'languages and percentages JSON response' do - let(:expected_languages) { project.repository.languages.map { |language| language.values_at(:label, :value)}.to_h } + let(:expected_languages) { project.repository.languages.to_h { |language| language.values_at(:label, :value) } } before do allow(project.repository).to receive(:languages).and_return( @@ -810,6 +810,54 @@ RSpec.describe API::Projects do end end end + + context 'with forked projects', :use_clean_rails_memory_store_caching do + include ProjectForksHelper + + let_it_be(:admin) { create(:admin) } + + it 'avoids N+1 queries' do + get api('/projects', admin) + + base_project = create(:project, :public, namespace: admin.namespace) + + fork_project1 = fork_project(base_project, admin, namespace: create(:user).namespace) + fork_project2 = fork_project(fork_project1, admin, namespace: create(:user).namespace) + + control = ActiveRecord::QueryRecorder.new do + get api('/projects', admin) + end + + fork_project(fork_project2, admin, namespace: create(:user).namespace) + + expect do + get api('/projects', admin) + end.not_to exceed_query_limit(control.count) + end + end + + context 'when service desk is enabled', :use_clean_rails_memory_store_caching do + let_it_be(:admin) { create(:admin) } + + it 'avoids N+1 queries' do + allow(Gitlab::ServiceDeskEmail).to receive(:enabled?).and_return(true) + allow(Gitlab::IncomingEmail).to receive(:enabled?).and_return(true) + + get api('/projects', admin) + + create(:project, :public, :service_desk_enabled, namespace: admin.namespace) + + control = ActiveRecord::QueryRecorder.new do + get api('/projects', admin) + end + + create_list(:project, 2, :public, :service_desk_enabled, namespace: admin.namespace) + + expect do + get api('/projects', admin) + end.not_to exceed_query_limit(control.count) + end + end end describe 'POST /projects' do @@ -1461,21 +1509,139 @@ RSpec.describe API::Projects do end end + describe "POST /projects/:id/uploads/authorize" do + include WorkhorseHelpers + + let(:headers) { workhorse_internal_api_request_header.merge({ 'HTTP_GITLAB_WORKHORSE' => 1 }) } + + context 'with authorized user' do + it "returns 200" do + post api("/projects/#{project.id}/uploads/authorize", user), headers: headers + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['MaximumSize']).to eq(project.max_attachment_size) + end + end + + context 'with unauthorized user' do + it "returns 404" do + post api("/projects/#{project.id}/uploads/authorize", user2), headers: headers + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'with exempted project' do + before do + stub_env('GITLAB_UPLOAD_API_ALLOWLIST', project.id) + end + + it "returns 200" do + post api("/projects/#{project.id}/uploads/authorize", user), headers: headers + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['MaximumSize']).to eq(1.gigabyte) + end + end + + context 'with upload size enforcement disabled' do + before do + stub_feature_flags(enforce_max_attachment_size_upload_api: false) + end + + it "returns 200" do + post api("/projects/#{project.id}/uploads/authorize", user), headers: headers + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['MaximumSize']).to eq(1.gigabyte) + end + end + + context 'with no Workhorse headers' do + it "returns 403" do + post api("/projects/#{project.id}/uploads/authorize", user) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + end + describe "POST /projects/:id/uploads" do + let(:file) { fixture_file_upload("spec/fixtures/dk.png", "image/png") } + before do project end it "uploads the file and returns its info" do - post api("/projects/#{project.id}/uploads", user), params: { file: fixture_file_upload("spec/fixtures/dk.png", "image/png") } + expect_next_instance_of(UploadService) do |instance| + expect(instance).to receive(:override_max_attachment_size=).with(project.max_attachment_size).and_call_original + end + + post api("/projects/#{project.id}/uploads", user), params: { file: file } expect(response).to have_gitlab_http_status(:created) expect(json_response['alt']).to eq("dk") expect(json_response['url']).to start_with("/uploads/") expect(json_response['url']).to end_with("/dk.png") - expect(json_response['full_path']).to start_with("/#{project.namespace.path}/#{project.path}/uploads") end + + it "does not leave the temporary file in place after uploading, even when the tempfile reaper does not run" do + stub_env('GITLAB_TEMPFILE_IMMEDIATE_UNLINK', '1') + tempfile = Tempfile.new('foo') + path = tempfile.path + + allow_any_instance_of(Rack::TempfileReaper).to receive(:call) do |instance, env| + instance.instance_variable_get(:@app).call(env) + end + + expect(path).not_to be(nil) + expect(Rack::Multipart::Parser::TEMPFILE_FACTORY).to receive(:call).and_return(tempfile) + + post api("/projects/#{project.id}/uploads", user), params: { file: fixture_file_upload("spec/fixtures/dk.png", "image/png") } + + expect(tempfile.path).to be(nil) + expect(File.exist?(path)).to be(false) + end + + shared_examples 'capped upload attachments' do |upload_allowed| + it "limits the upload to 1 GB" do + expect_next_instance_of(UploadService) do |instance| + expect(instance).to receive(:override_max_attachment_size=).with(1.gigabyte).and_call_original + end + + post api("/projects/#{project.id}/uploads", user), params: { file: file } + + expect(response).to have_gitlab_http_status(:created) + end + + it "logs a warning if file exceeds attachment size" do + allow(Gitlab::CurrentSettings).to receive(:max_attachment_size).and_return(0) + + expect(Gitlab::AppLogger).to receive(:info).with( + hash_including(message: 'File exceeds maximum size', upload_allowed: upload_allowed)) + .and_call_original + + post api("/projects/#{project.id}/uploads", user), params: { file: file } + end + end + + context 'with exempted project' do + before do + stub_env('GITLAB_UPLOAD_API_ALLOWLIST', project.id) + end + + it_behaves_like 'capped upload attachments', true + end + + context 'with upload size enforcement disabled' do + before do + stub_feature_flags(enforce_max_attachment_size_upload_api: false) + end + + it_behaves_like 'capped upload attachments', false + end end describe "GET /projects/:id/groups" do diff --git a/spec/requests/api/pypi_packages_spec.rb b/spec/requests/api/pypi_packages_spec.rb index ae5b132f409..718004a0087 100644 --- a/spec/requests/api/pypi_packages_spec.rb +++ b/spec/requests/api/pypi_packages_spec.rb @@ -118,7 +118,7 @@ RSpec.describe API::PypiPackages do it_behaves_like 'deploy token for package uploads' - it_behaves_like 'job token for package uploads' + it_behaves_like 'job token for package uploads', authorize_endpoint: true it_behaves_like 'rejects PyPI access with unknown project id' end diff --git a/spec/requests/api/repositories_spec.rb b/spec/requests/api/repositories_spec.rb index 31f0d7cec2a..a12b4dc9848 100644 --- a/spec/requests/api/repositories_spec.rb +++ b/spec/requests/api/repositories_spec.rb @@ -6,6 +6,7 @@ require 'mime/types' RSpec.describe API::Repositories do include RepoHelpers include WorkhorseHelpers + include ProjectForksHelper let(:user) { create(:user) } let(:guest) { create(:user).tap { |u| create(:project_member, :guest, user: u, project: project) } } @@ -392,6 +393,28 @@ RSpec.describe API::Repositories do expect(json_response['diffs']).to be_present end + it "compare commits between different projects with non-forked relation" do + public_project = create(:project, :repository, :public) + + get api(route, current_user), params: { from: sample_commit.parent_id, to: sample_commit.id, from_project_id: public_project.id } + + expect(response).to have_gitlab_http_status(:bad_request) + end + + it "compare commits between different projects" do + group = create(:group) + group.add_owner(current_user) + + forked_project = fork_project(project, current_user, repository: true, namespace: group) + forked_project.repository.create_ref('refs/heads/improve/awesome', 'refs/heads/improve/more-awesome') + + get api(route, current_user), params: { from: 'improve/awesome', to: 'feature', from_project_id: forked_project.id } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['commits']).to be_present + expect(json_response['diffs']).to be_present + end + it "compares same refs" do get api(route, current_user), params: { from: 'master', to: 'master' } diff --git a/spec/requests/api/resource_access_tokens_spec.rb b/spec/requests/api/resource_access_tokens_spec.rb index 79549bfc5e0..1a3c805fe9f 100644 --- a/spec/requests/api/resource_access_tokens_spec.rb +++ b/spec/requests/api/resource_access_tokens_spec.rb @@ -151,6 +151,23 @@ RSpec.describe API::ResourceAccessTokens do expect(User.exists?(project_bot.id)).to be_falsy end + context "when using project access token to DELETE other project access token" do + let_it_be(:other_project_bot) { create(:user, :project_bot) } + let_it_be(:other_token) { create(:personal_access_token, user: other_project_bot) } + let_it_be(:token_id) { other_token.id } + + before do + project.add_maintainer(other_project_bot) + end + + it "deletes the project access token from the project" do + delete_token + + expect(response).to have_gitlab_http_status(:no_content) + expect(User.exists?(other_project_bot.id)).to be_falsy + end + end + context "when attempting to delete a non-existent project access token" do let_it_be(:token_id) { non_existing_record_id } diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index 3b84c812010..48f5bd114a1 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe API::Settings, 'Settings' do +RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting do let(:user) { create(:user) } let_it_be(:admin) { create(:admin) } @@ -44,6 +44,7 @@ RSpec.describe API::Settings, 'Settings' do expect(json_response['wiki_page_max_content_bytes']).to be_a(Integer) expect(json_response['require_admin_approval_after_user_signup']).to eq(true) expect(json_response['personal_access_token_prefix']).to be_nil + expect(json_response['admin_mode']).to be(false) end end @@ -124,7 +125,8 @@ RSpec.describe API::Settings, 'Settings' do disabled_oauth_sign_in_sources: 'unknown', import_sources: 'github,bitbucket', wiki_page_max_content_bytes: 12345, - personal_access_token_prefix: "GL-" + personal_access_token_prefix: "GL-", + admin_mode: true } expect(response).to have_gitlab_http_status(:ok) @@ -169,6 +171,7 @@ RSpec.describe API::Settings, 'Settings' do expect(json_response['import_sources']).to match_array(%w(github bitbucket)) expect(json_response['wiki_page_max_content_bytes']).to eq(12345) expect(json_response['personal_access_token_prefix']).to eq("GL-") + expect(json_response['admin_mode']).to be(true) end end diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb index b029c0f5793..3c698cf577e 100644 --- a/spec/requests/api/tags_spec.rb +++ b/spec/requests/api/tags_spec.rb @@ -17,131 +17,197 @@ RSpec.describe API::Tags do end describe 'GET /projects/:id/repository/tags' do - let(:route) { "/projects/#{project_id}/repository/tags" } + shared_examples "get repository tags" do + let(:route) { "/projects/#{project_id}/repository/tags" } - context 'sorting' do - let(:current_user) { user } + context 'sorting' do + let(:current_user) { user } - it 'sorts by descending order by default' do - get api(route, current_user) + it 'sorts by descending order by default' do + get api(route, current_user) - desc_order_tags = project.repository.tags.sort_by { |tag| tag.dereferenced_target.committed_date } - desc_order_tags.reverse!.map! { |tag| tag.dereferenced_target.id } + desc_order_tags = project.repository.tags.sort_by { |tag| tag.dereferenced_target.committed_date } + desc_order_tags.reverse!.map! { |tag| tag.dereferenced_target.id } - expect(json_response.map { |tag| tag['commit']['id'] }).to eq(desc_order_tags) - end + expect(json_response.map { |tag| tag['commit']['id'] }).to eq(desc_order_tags) + end - it 'sorts by ascending order if specified' do - get api("#{route}?sort=asc", current_user) + it 'sorts by ascending order if specified' do + get api("#{route}?sort=asc", current_user) - asc_order_tags = project.repository.tags.sort_by { |tag| tag.dereferenced_target.committed_date } - asc_order_tags.map! { |tag| tag.dereferenced_target.id } + asc_order_tags = project.repository.tags.sort_by { |tag| tag.dereferenced_target.committed_date } + asc_order_tags.map! { |tag| tag.dereferenced_target.id } - expect(json_response.map { |tag| tag['commit']['id'] }).to eq(asc_order_tags) - end + expect(json_response.map { |tag| tag['commit']['id'] }).to eq(asc_order_tags) + end - it 'sorts by name in descending order when requested' do - get api("#{route}?order_by=name", current_user) + it 'sorts by name in descending order when requested' do + get api("#{route}?order_by=name", current_user) - ordered_by_name = project.repository.tags.map { |tag| tag.name }.sort.reverse + ordered_by_name = project.repository.tags.map { |tag| tag.name }.sort.reverse - expect(json_response.map { |tag| tag['name'] }).to eq(ordered_by_name) - end + expect(json_response.map { |tag| tag['name'] }).to eq(ordered_by_name) + end - it 'sorts by name in ascending order when requested' do - get api("#{route}?order_by=name&sort=asc", current_user) + it 'sorts by name in ascending order when requested' do + get api("#{route}?order_by=name&sort=asc", current_user) - ordered_by_name = project.repository.tags.map { |tag| tag.name }.sort + ordered_by_name = project.repository.tags.map { |tag| tag.name }.sort - expect(json_response.map { |tag| tag['name'] }).to eq(ordered_by_name) + expect(json_response.map { |tag| tag['name'] }).to eq(ordered_by_name) + end end - end - context 'searching' do - it 'only returns searched tags' do - get api("#{route}", user), params: { search: 'v1.1.0' } + context 'searching' do + it 'only returns searched tags' do + get api("#{route}", user), params: { search: 'v1.1.0' } - expect(response).to have_gitlab_http_status(:ok) - expect(response).to include_pagination_headers - expect(json_response).to be_an Array - expect(json_response.size).to eq(1) - expect(json_response[0]['name']).to eq('v1.1.0') + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.size).to eq(1) + expect(json_response[0]['name']).to eq('v1.1.0') + end end - end - shared_examples_for 'repository tags' do - it 'returns the repository tags' do - get api(route, current_user) + shared_examples_for 'repository tags' do + it 'returns the repository tags' do + get api(route, current_user) - expect(response).to have_gitlab_http_status(:ok) - expect(response).to match_response_schema('public_api/v4/tags') - expect(response).to include_pagination_headers - expect(json_response.map { |r| r['name'] }).to include(tag_name) + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/tags') + expect(response).to include_pagination_headers + expect(json_response.map { |r| r['name'] }).to include(tag_name) + end + + context 'when repository is disabled' do + include_context 'disabled repository' + + it_behaves_like '403 response' do + let(:request) { get api(route, current_user) } + end + end end - context 'when repository is disabled' do - include_context 'disabled repository' + context 'when unauthenticated', 'and project is public' do + let(:project) { create(:project, :public, :repository) } - it_behaves_like '403 response' do - let(:request) { get api(route, current_user) } + it_behaves_like 'repository tags' + end + + context 'when unauthenticated', 'and project is private' do + it_behaves_like '404 response' do + let(:request) { get api(route) } + let(:message) { '404 Project Not Found' } end end - end - context 'when unauthenticated', 'and project is public' do - let(:project) { create(:project, :public, :repository) } + context 'when authenticated', 'as a maintainer' do + let(:current_user) { user } - it_behaves_like 'repository tags' - end + it_behaves_like 'repository tags' - context 'when unauthenticated', 'and project is private' do - it_behaves_like '404 response' do - let(:request) { get api(route) } - let(:message) { '404 Project Not Found' } + context 'requesting with the escaped project full path' do + let(:project_id) { CGI.escape(project.full_path) } + + it_behaves_like 'repository tags' + end end - end - context 'when authenticated', 'as a maintainer' do - let(:current_user) { user } + context 'when authenticated', 'as a guest' do + it_behaves_like '403 response' do + let(:request) { get api(route, guest) } + end + end - it_behaves_like 'repository tags' + context 'with releases' do + let(:description) { 'Awesome release!' } - context 'requesting with the escaped project full path' do - let(:project_id) { CGI.escape(project.full_path) } + let!(:release) do + create(:release, + :legacy, + project: project, + tag: tag_name, + description: description) + end - it_behaves_like 'repository tags' + it 'returns an array of project tags with release info' do + get api(route, user) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/tags') + expect(response).to include_pagination_headers + + expected_tag = json_response.find { |r| r['name'] == tag_name } + expect(expected_tag['message']).to eq(tag_message) + expect(expected_tag['release']['description']).to eq(description) + end end end - context 'when authenticated', 'as a guest' do - it_behaves_like '403 response' do - let(:request) { get api(route, guest) } + context ":api_caching_tags flag enabled", :use_clean_rails_memory_store_caching do + before do + stub_feature_flags(api_caching_tags: true) end - end - context 'with releases' do - let(:description) { 'Awesome release!' } + it_behaves_like "get repository tags" - let!(:release) do - create(:release, - :legacy, - project: project, - tag: tag_name, - description: description) - end + describe "cache expiry" do + let(:route) { "/projects/#{project_id}/repository/tags" } + let(:current_user) { user } - it 'returns an array of project tags with release info' do - get api(route, user) + before do + # Set the cache + get api(route, current_user) + end - expect(response).to have_gitlab_http_status(:ok) - expect(response).to match_response_schema('public_api/v4/tags') - expect(response).to include_pagination_headers + it "is cached" do + expect(API::Entities::Tag).not_to receive(:represent) + + get api(route, current_user) + end + + shared_examples "cache expired" do + it "isn't cached" do + expect(API::Entities::Tag).to receive(:represent).exactly(3).times + + get api(route, current_user) + end + end + + context "when protected tag is changed" do + before do + create(:protected_tag, name: tag_name, project: project) + end + + it_behaves_like "cache expired" + end - expected_tag = json_response.find { |r| r['name'] == tag_name } - expect(expected_tag['message']).to eq(tag_message) - expect(expected_tag['release']['description']).to eq(description) + context "when release is changed" do + before do + create(:release, :legacy, project: project, tag: tag_name) + end + + it_behaves_like "cache expired" + end + + context "when project is changed" do + before do + project.touch + end + + it_behaves_like "cache expired" + end end end + + context ":api_caching_tags flag disabled" do + before do + stub_feature_flags(api_caching_tags: false) + end + + it_behaves_like "get repository tags" + end end describe 'GET /projects/:id/repository/tags/:tag_name' do diff --git a/spec/requests/api/triggers_spec.rb b/spec/requests/api/triggers_spec.rb index 55d17fabc9a..4318f106996 100644 --- a/spec/requests/api/triggers_spec.rb +++ b/spec/requests/api/triggers_spec.rb @@ -49,8 +49,6 @@ RSpec.describe API::Triggers do expect(response).to have_gitlab_http_status(:created) expect(json_response).to include('id' => pipeline.id) - pipeline.builds.reload - expect(pipeline.builds.pending.size).to eq(2) expect(pipeline.builds.size).to eq(5) end @@ -126,6 +124,39 @@ RSpec.describe API::Triggers do end end + describe 'adding arguments to the application context' do + subject { subject_proc.call } + + let(:expected_params) { { client_id: "user/#{user.id}", project: project.full_path } } + let(:subject_proc) { proc { post api("/projects/#{project.id}/ref/master/trigger/pipeline?token=#{trigger_token}"), params: { ref: 'refs/heads/other-branch' } } } + + context 'when triggering a pipeline from a trigger token' do + it_behaves_like 'storing arguments in the application context' + it_behaves_like 'not executing any extra queries for the application context' + end + + context 'when triggered from another running job' do + let!(:trigger) { } + let!(:trigger_request) { } + + context 'when other job is triggered by a user' do + let(:trigger_token) { create(:ci_build, :running, project: project, user: user).token } + + it_behaves_like 'storing arguments in the application context' + it_behaves_like 'not executing any extra queries for the application context' + end + + context 'when other job is triggered by a runner' do + let(:trigger_token) { create(:ci_build, :running, project: project, runner: runner).token } + let(:runner) { create(:ci_runner) } + let(:expected_params) { { client_id: "runner/#{runner.id}", project: project.full_path } } + + it_behaves_like 'storing arguments in the application context' + it_behaves_like 'not executing any extra queries for the application context', 1 + end + end + end + context 'when is triggered by a pipeline hook' do it 'does not create a new pipeline' do expect do diff --git a/spec/requests/api/usage_data_non_sql_metrics_spec.rb b/spec/requests/api/usage_data_non_sql_metrics_spec.rb new file mode 100644 index 00000000000..225af57a267 --- /dev/null +++ b/spec/requests/api/usage_data_non_sql_metrics_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe API::UsageDataNonSqlMetrics do + include UsageDataHelpers + + let_it_be(:admin) { create(:user, admin: true) } + let_it_be(:user) { create(:user) } + + before do + stub_usage_data_connections + end + + describe 'GET /usage_data/non_sql_metrics' do + let(:endpoint) { '/usage_data/non_sql_metrics' } + + context 'with authentication' do + before do + stub_feature_flags(usage_data_non_sql_metrics: true) + end + + it 'returns non sql metrics if user is admin' do + get api(endpoint, admin) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['counts']).to be_a(Hash) + end + + it 'returns forbidden if user is not admin' do + get api(endpoint, user) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + + context 'without authentication' do + before do + stub_feature_flags(usage_data_non_sql_metrics: true) + end + + it 'returns unauthorized' do + get api(endpoint) + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context 'when feature_flag is disabled' do + before do + stub_feature_flags(usage_data_non_sql_metrics: false) + end + + it 'returns not_found for admin' do + get api(endpoint, admin) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'returns forbidden for non-admin' do + get api(endpoint, user) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + end +end diff --git a/spec/requests/api/usage_data_queries_spec.rb b/spec/requests/api/usage_data_queries_spec.rb new file mode 100644 index 00000000000..0ba4a37bc9b --- /dev/null +++ b/spec/requests/api/usage_data_queries_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe API::UsageDataQueries do + include UsageDataHelpers + + let_it_be(:admin) { create(:user, admin: true) } + let_it_be(:user) { create(:user) } + + before do + stub_usage_data_connections + end + + describe 'GET /usage_data/usage_data_queries' do + let(:endpoint) { '/usage_data/queries' } + + context 'with authentication' do + before do + stub_feature_flags(usage_data_queries_api: true) + end + + it 'returns queries if user is admin' do + get api(endpoint, admin) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['active_user_count']).to start_with('SELECT COUNT("users"."id") FROM "users"') + end + + it 'returns forbidden if user is not admin' do + get api(endpoint, user) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + + context 'without authentication' do + before do + stub_feature_flags(usage_data_queries_api: true) + end + + it 'returns unauthorized' do + get api(endpoint) + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context 'when feature_flag is disabled' do + before do + stub_feature_flags(usage_data_queries_api: false) + end + + it 'returns not_found for admin' do + get api(endpoint, admin) + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'returns forbidden for non-admin' do + get api(endpoint, user) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + end +end diff --git a/spec/requests/api/usage_data_spec.rb b/spec/requests/api/usage_data_spec.rb index d44f179eed8..bacaf960e6a 100644 --- a/spec/requests/api/usage_data_spec.rb +++ b/spec/requests/api/usage_data_spec.rb @@ -161,4 +161,23 @@ RSpec.describe API::UsageData do end end end + + describe 'GET /usage_data/metric_definitions' do + let(:endpoint) { '/usage_data/metric_definitions' } + let(:metric_yaml) do + { 'key_path' => 'counter.category.event', 'description' => 'Metric description' }.to_yaml + end + + context 'without authentication' do + it 'returns a YAML file', :aggregate_failures do + allow(Gitlab::Usage::MetricDefinition).to receive(:dump_metrics_yaml).and_return(metric_yaml) + + get api(endpoint) + + expect(response).to have_gitlab_http_status(:ok) + expect(response.media_type).to eq('application/yaml') + expect(response.body).to eq(metric_yaml) + end + end + end end diff --git a/spec/requests/api/users_preferences_spec.rb b/spec/requests/api/users_preferences_spec.rb new file mode 100644 index 00000000000..db03786ed2a --- /dev/null +++ b/spec/requests/api/users_preferences_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe API::Users do + let_it_be(:user) { create(:user) } + + describe 'PUT /user/preferences/' do + context "with correct attributes and a logged in user" do + it 'returns a success status and the value has been changed' do + put api("/user/preferences", user), params: { view_diffs_file_by_file: true } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['view_diffs_file_by_file']).to eq(true) + expect(user.reload.view_diffs_file_by_file).to be_truthy + end + end + + context "missing a preference" do + it 'returns a bad request status' do + put api("/user/preferences", user), params: {} + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + + context "without a logged in user" do + it 'returns an unauthorized status' do + put api("/user/preferences"), params: { view_diffs_file_by_file: true } + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context "with an unsupported preference" do + it 'returns a bad parameter' do + put api("/user/preferences", user), params: { jawn: true } + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + + context "with an unsupported value" do + it 'returns a bad parameter' do + put api("/user/preferences", user), params: { view_diffs_file_by_file: 3 } + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + + context "with an update service failure" do + it 'returns a bad request' do + bad_service = double("Failed Service", success?: false) + + allow_next_instance_of(::UserPreferences::UpdateService) do |instance| + allow(instance).to receive(:execute).and_return(bad_service) + end + + put api("/user/preferences", user), params: { view_diffs_file_by_file: true } + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + end +end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 2a7689eaddf..01a24be9f20 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -928,7 +928,8 @@ RSpec.describe API::Users do end it "creates user with random password" do - params = attributes_for(:user, force_random_password: true, reset_password: true) + params = attributes_for(:user, force_random_password: true) + params.delete(:password) post api('/users', admin), params: params expect(response).to have_gitlab_http_status(:created) @@ -936,8 +937,7 @@ RSpec.describe API::Users do user_id = json_response['id'] new_user = User.find(user_id) - expect(new_user.valid_password?(params[:password])).to eq(false) - expect(new_user.recently_sent_password_reset?).to eq(true) + expect(new_user.encrypted_password).to be_present end it "creates user with private profile" do @@ -1795,8 +1795,7 @@ RSpec.describe API::Users do post api("/users/#{user.id}/emails", admin), params: email_attrs end.to change { user.emails.count }.by(1) - email = Email.find_by(user_id: user.id, email: email_attrs[:email]) - expect(email).not_to be_confirmed + expect(json_response['confirmed_at']).to be_nil end it "returns a 400 for invalid ID" do @@ -1813,8 +1812,7 @@ RSpec.describe API::Users do expect(response).to have_gitlab_http_status(:created) - email = Email.find_by(user_id: user.id, email: email_attrs[:email]) - expect(email).to be_confirmed + expect(json_response['confirmed_at']).not_to be_nil end end diff --git a/spec/requests/api/v3/github_spec.rb b/spec/requests/api/v3/github_spec.rb index 197c6cbb0eb..4100b246218 100644 --- a/spec/requests/api/v3/github_spec.rb +++ b/spec/requests/api/v3/github_spec.rb @@ -3,10 +3,10 @@ require 'spec_helper' RSpec.describe API::V3::Github do - let(:user) { create(:user) } - let(:unauthorized_user) { create(:user) } - let(:admin) { create(:user, :admin) } - let(:project) { create(:project, :repository, creator: user) } + let_it_be(:user) { create(:user) } + let_it_be(:unauthorized_user) { create(:user) } + let_it_be(:admin) { create(:user, :admin) } + let_it_be(:project) { create(:project, :repository, creator: user) } before do project.add_maintainer(user) @@ -210,14 +210,14 @@ RSpec.describe API::V3::Github do end describe 'repo pulls' do - let(:project2) { create(:project, :repository, creator: user) } - let(:assignee) { create(:user) } - let(:assignee2) { create(:user) } - let!(:merge_request) do + let_it_be(:project2) { create(:project, :repository, creator: user) } + let_it_be(:assignee) { create(:user) } + let_it_be(:assignee2) { create(:user) } + let_it_be(:merge_request) do create(:merge_request, source_project: project, target_project: project, author: user, assignees: [assignee]) end - let!(:merge_request_2) do + let_it_be(:merge_request_2) do create(:merge_request, source_project: project2, target_project: project2, author: user, assignees: [assignee, assignee2]) end @@ -225,26 +225,57 @@ RSpec.describe API::V3::Github do project2.add_maintainer(user) end + def perform_request + jira_get v3_api(route, user) + end + describe 'GET /-/jira/pulls' do + let(:route) { '/repos/-/jira/pulls' } + it 'returns an array of merge requests with github format' do - jira_get v3_api('/repos/-/jira/pulls', user) + perform_request expect(response).to have_gitlab_http_status(:ok) expect(json_response).to be_an(Array) expect(json_response.size).to eq(2) expect(response).to match_response_schema('entities/github/pull_requests') end + + it 'returns multiple merge requests without N + 1' do + perform_request + + control_count = ActiveRecord::QueryRecorder.new { perform_request }.count + + project3 = create(:project, :repository, creator: user) + project3.add_maintainer(user) + assignee3 = create(:user) + create(:merge_request, source_project: project3, target_project: project3, author: user, assignees: [assignee3]) + + expect { perform_request }.not_to exceed_query_limit(control_count) + end end describe 'GET /repos/:namespace/:project/pulls' do + let(:route) { "/repos/#{project.namespace.path}/#{project.path}/pulls" } + it 'returns an array of merge requests for the proper project in github format' do - jira_get v3_api("/repos/#{project.namespace.path}/#{project.path}/pulls", user) + perform_request expect(response).to have_gitlab_http_status(:ok) expect(json_response).to be_an(Array) expect(json_response.size).to eq(1) expect(response).to match_response_schema('entities/github/pull_requests') end + + it 'returns multiple merge requests without N + 1' do + perform_request + + control_count = ActiveRecord::QueryRecorder.new { perform_request }.count + + create(:merge_request, source_project: project, source_branch: 'fix') + + expect { perform_request }.not_to exceed_query_limit(control_count) + end end describe 'GET /repos/:namespace/:project/pulls/:id' do |