From 2a6b543cbf045b7d480a21c1c77fd4f0243a6437 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Tue, 30 Nov 2021 03:13:04 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .rubocop_manual_todo.yml | 2 - app/graphql/types/ci/pipeline_type.rb | 8 +- app/graphql/types/ci/stage_type.rb | 2 +- app/graphql/types/project_type.rb | 2 +- .../models/concerns}/after_commit_queue.rb | 2 +- .../forbid_sidekiq_in_transactions.rb | 8 +- qa/qa/resource/project.rb | 15 +- .../api/1_manage/bulk_import_project_spec.rb | 187 ------------------ .../forbid_sidekiq_in_transactions_spec.rb | 38 ++++ .../concerns}/after_commit_queue_spec.rb | 2 +- .../requests/api/graphql/ci/pipelines_spec.rb | 12 +- .../api/graphql/project_query_spec.rb | 34 ++++ spec/spec_helper.rb | 2 + 13 files changed, 98 insertions(+), 216 deletions(-) rename {lib => app/models/concerns}/after_commit_queue.rb (94%) delete mode 100644 qa/qa/specs/features/api/1_manage/bulk_import_project_spec.rb create mode 100644 spec/initializers/forbid_sidekiq_in_transactions_spec.rb rename spec/{lib => models/concerns}/after_commit_queue_spec.rb (94%) diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index 73fbbcb3909..64172533f68 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -90,7 +90,6 @@ Rails/SaveBang: - 'spec/controllers/boards/issues_controller_spec.rb' - 'spec/controllers/sent_notifications_controller_spec.rb' - 'spec/controllers/sessions_controller_spec.rb' - - 'spec/lib/after_commit_queue_spec.rb' - 'spec/lib/backup/manager_spec.rb' - 'spec/lib/gitlab/alerting/alert_spec.rb' - 'spec/lib/gitlab/analytics/cycle_analytics/records_fetcher_spec.rb' @@ -2392,7 +2391,6 @@ Database/MultipleDatabases: - 'ee/spec/models/pg_replication_slot_spec.rb' - 'ee/spec/services/ee/merge_requests/update_service_spec.rb' - 'lib/backup/database.rb' - - 'lib/after_commit_queue.rb' - 'lib/backup/manager.rb' - 'lib/gitlab/current_settings.rb' - 'lib/gitlab/database/load_balancing/load_balancer.rb' diff --git a/app/graphql/types/ci/pipeline_type.rb b/app/graphql/types/ci/pipeline_type.rb index da2f11be9e2..1bbda1850bc 100644 --- a/app/graphql/types/ci/pipeline_type.rb +++ b/app/graphql/types/ci/pipeline_type.rb @@ -66,7 +66,7 @@ module Types field :stages, type: Types::Ci::StageType.connection_type, null: true, - authorize: :read_commit_status, + authorize: :read_build, description: 'Stages of the pipeline.', extras: [:lookahead], resolver: Resolvers::Ci::PipelineStagesResolver @@ -89,14 +89,14 @@ module Types field :jobs, ::Types::Ci::JobType.connection_type, null: true, - authorize: :read_commit_status, + authorize: :read_build, description: 'Jobs belonging to the pipeline.', resolver: ::Resolvers::Ci::JobsResolver field :job, type: ::Types::Ci::JobType, null: true, - authorize: :read_commit_status, + authorize: :read_build, description: 'Specific job in this pipeline, either by name or ID.' do argument :id, type: ::Types::GlobalIDType[::CommitStatus], @@ -116,7 +116,7 @@ module Types field :source_job, type: Types::Ci::JobType, null: true, - authorize: :read_commit_status, + authorize: :read_build, description: 'Job where pipeline was triggered from.' field :downstream, Types::Ci::PipelineType.connection_type, null: true, diff --git a/app/graphql/types/ci/stage_type.rb b/app/graphql/types/ci/stage_type.rb index fc887c83da3..70e78e391a7 100644 --- a/app/graphql/types/ci/stage_type.rb +++ b/app/graphql/types/ci/stage_type.rb @@ -4,7 +4,7 @@ module Types module Ci class StageType < BaseObject graphql_name 'CiStage' - authorize :read_commit_status + authorize :read_build field :id, GraphQL::Types::ID, null: false, description: 'ID of the stage.' diff --git a/app/graphql/types/project_type.rb b/app/graphql/types/project_type.rb index 8ef53ae001b..3d2ee47a499 100644 --- a/app/graphql/types/project_type.rb +++ b/app/graphql/types/project_type.rb @@ -194,7 +194,7 @@ module Types field :jobs, type: Types::Ci::JobType.connection_type, null: true, - authorize: :read_commit_status, + authorize: :read_build, description: 'Jobs of a project. This field can only be resolved for one project in any single request.', resolver: Resolvers::ProjectJobsResolver diff --git a/lib/after_commit_queue.rb b/app/models/concerns/after_commit_queue.rb similarity index 94% rename from lib/after_commit_queue.rb rename to app/models/concerns/after_commit_queue.rb index cbeaea97951..80c91658868 100644 --- a/lib/after_commit_queue.rb +++ b/app/models/concerns/after_commit_queue.rb @@ -16,7 +16,7 @@ module AfterCommitQueue def run_after_commit_or_now(&block) if ApplicationRecord.inside_transaction? - if ActiveRecord::Base.connection.current_transaction.records&.include?(self) + if ActiveRecord::Base.connection.current_transaction.records&.include?(self) # rubocop: disable Database/MultipleDatabases run_after_commit(&block) else # If the current transaction does not include this record, we can run diff --git a/config/initializers/forbid_sidekiq_in_transactions.rb b/config/initializers/forbid_sidekiq_in_transactions.rb index e5e17672c4e..2ea6c9a7343 100644 --- a/config/initializers/forbid_sidekiq_in_transactions.rb +++ b/config/initializers/forbid_sidekiq_in_transactions.rb @@ -20,7 +20,7 @@ module Sidekiq module NoEnqueueingFromTransactions %i(perform_async perform_at perform_in).each do |name| define_method(name) do |*args| - if !Sidekiq::Worker.skip_transaction_check && ApplicationRecord.inside_transaction? + if !Sidekiq::Worker.skip_transaction_check && inside_transaction? begin raise Sidekiq::Worker::EnqueueFromTransactionError, <<~MSG `#{self}.#{name}` cannot be called inside a transaction as this can lead to @@ -38,6 +38,12 @@ module Sidekiq super(*args) end end + + private + + def inside_transaction? + ::ApplicationRecord.inside_transaction? || ::Ci::ApplicationRecord.inside_transaction? + end end prepend NoEnqueueingFromTransactions diff --git a/qa/qa/resource/project.rb b/qa/qa/resource/project.rb index 424b17e413d..192c5e4b2db 100644 --- a/qa/qa/resource/project.rb +++ b/qa/qa/resource/project.rb @@ -217,10 +217,6 @@ module QA "#{api_get_path}/wikis" end - def api_push_rules_path - "#{api_get_path}/push_rule" - end - def api_post_body post_body = { name: name, @@ -372,15 +368,6 @@ module QA parse_body(response) end - def push_rules - response = get(request_url(api_push_rules_path)) - parse_body(response) - end - - def add_push_rules(rules) - api_post_to(api_push_rules_path, rules) - end - # Object comparison # # @param [QA::Resource::Project] other @@ -441,3 +428,5 @@ module QA end end end + +QA::Resource::Project.prepend_mod_with('Resource::Project', namespace: QA) diff --git a/qa/qa/specs/features/api/1_manage/bulk_import_project_spec.rb b/qa/qa/specs/features/api/1_manage/bulk_import_project_spec.rb deleted file mode 100644 index 43b9dc78806..00000000000 --- a/qa/qa/specs/features/api/1_manage/bulk_import_project_spec.rb +++ /dev/null @@ -1,187 +0,0 @@ -# frozen_string_literal: true - -module QA - # run only base UI validation on staging because test requires top level group creation which is problematic - # on staging environment - RSpec.describe 'Manage', :requires_admin, except: { subdomain: :staging } do - describe 'Bulk project import' do - let(:import_wait_duration) { { max_duration: 300, sleep_interval: 2 } } - let(:admin_api_client) { Runtime::API::Client.as_admin } - let(:user) do - Resource::User.fabricate_via_api! do |usr| - usr.api_client = admin_api_client - usr.hard_delete_on_api_removal = true - end - end - - let(:api_client) { Runtime::API::Client.new(user: user) } - - let(:sandbox) do - Resource::Sandbox.fabricate_via_api! do |group| - group.api_client = admin_api_client - end - end - - let(:source_group) do - Resource::Sandbox.fabricate_via_api! do |group| - group.api_client = api_client - group.path = "source-group-for-import-#{SecureRandom.hex(4)}" - end - end - - let(:source_project) do - Resource::Project.fabricate_via_api! do |project| - project.api_client = api_client - project.group = source_group - project.initialize_with_readme = true - end - end - - let(:imported_group) do - Resource::BulkImportGroup.fabricate_via_api! do |group| - group.api_client = api_client - group.sandbox = sandbox - group.source_group_path = source_group.path - end - end - - let(:imported_projects) do - imported_group.reload!.projects - end - - let(:project_import_failures) do - imported_group.import_details - .find { |entity| entity[:destination_name] == source_project.name } - &.fetch(:failures) - end - - before do - Runtime::Feature.enable(:bulk_import_projects) - - sandbox.add_member(user, Resource::Members::AccessLevel::MAINTAINER) - - source_project.tap { |project| project.add_push_rules(member_check: true) } # fabricate source group and project - end - - after do - user.remove_via_api! - ensure - Runtime::Feature.disable(:bulk_import_projects) - end - - context 'with project' do - before do - imported_group # trigger import - end - - it( - 'successfully imports project', - testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/quality/test_cases/2297' - ) do - expect { imported_group.import_status }.to eventually_eq('finished').within(import_wait_duration) - expect(imported_projects.count).to eq(1), 'Expected to have 1 imported project' - - aggregate_failures do - expect(imported_projects.first).to eq(source_project) - expect(project_import_failures).to be_empty, "Expected no errors, was: #{project_import_failures}" - end - end - end - - context 'with project issues' do - let(:source_issue) do - Resource::Issue.fabricate_via_api! do |issue| - issue.api_client = api_client - issue.project = source_project - issue.labels = %w[label_one label_two] - end - end - - let(:imported_issues) do - imported_projects.first.issues - end - - let(:imported_issue) do - issue = imported_issues.first - Resource::Issue.init do |resource| - resource.api_client = api_client - resource.project = imported_projects.first - resource.iid = issue[:iid] - end - end - - before do - source_issue # fabricate source group, project, issue - imported_group # trigger import - end - - it( - 'successfully imports issue', - testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/quality/test_cases/2325' - ) do - expect { imported_group.import_status }.to eventually_eq('finished').within(import_wait_duration) - expect(imported_projects.count).to eq(1), 'Expected to have 1 imported project' - - aggregate_failures do - expect(imported_issues.count).to eq(1) - expect(imported_issue.reload!).to eq(source_issue) - expect(project_import_failures).to be_empty, "Expected no errors, was: #{project_import_failures}" - end - end - end - - context 'with repository' do - let(:imported_project) { imported_projects.first } - - let(:source_commits) { source_project.commits.map { |c| c.except(:web_url) } } - let(:source_tags) do - source_project.repository_tags.tap do |tags| - tags.each { |t| t[:commit].delete(:web_url) } - end - end - - let(:source_branches) do - source_project.repository_branches.tap do |branches| - branches.each do |b| - b.delete(:web_url) - b[:commit].delete(:web_url) - end - end - end - - let(:imported_commits) { imported_project.commits.map { |c| c.except(:web_url) } } - let(:imported_tags) do - imported_project.repository_tags.tap do |tags| - tags.each { |t| t[:commit].delete(:web_url) } - end - end - - let(:imported_branches) do - imported_project.repository_branches.tap do |branches| - branches.each do |b| - b.delete(:web_url) - b[:commit].delete(:web_url) - end - end - end - - before do - source_project.create_repository_branch('test-branch') - source_project.create_repository_tag('v0.0.1') - imported_group # trigger import - end - - it 'successfully imports repository' do - expect { imported_group.import_status }.to eventually_eq('finished').within(import_wait_duration) - expect(imported_projects.count).to eq(1), 'Expected to have 1 imported project' - - aggregate_failures do - expect(imported_commits).to match_array(source_commits) - expect(imported_tags).to match_array(source_tags) - expect(imported_branches).to match_array(source_branches) - end - end - end - end - end -end diff --git a/spec/initializers/forbid_sidekiq_in_transactions_spec.rb b/spec/initializers/forbid_sidekiq_in_transactions_spec.rb new file mode 100644 index 00000000000..6cd15d37ad4 --- /dev/null +++ b/spec/initializers/forbid_sidekiq_in_transactions_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Sidekiq::Worker' do + let(:worker_class) do + Class.new do + include Sidekiq::Worker + + def perform + end + end + end + + it 'allows sidekiq worker outside of a transaction' do + expect { worker_class.perform_async }.not_to raise_error + end + + it 'forbids queue sidekiq worker in a transaction' do + Project.transaction do + expect { worker_class.perform_async }.to raise_error(Sidekiq::Worker::EnqueueFromTransactionError) + end + end + + it 'allows sidekiq worker in a transaction if skipped' do + Sidekiq::Worker.skipping_transaction_check do + Project.transaction do + expect { worker_class.perform_async }.not_to raise_error + end + end + end + + it 'forbids queue sidekiq worker in a Ci::ApplicationRecord transaction' do + Ci::Pipeline.transaction do + expect { worker_class.perform_async }.to raise_error(Sidekiq::Worker::EnqueueFromTransactionError) + end + end +end diff --git a/spec/lib/after_commit_queue_spec.rb b/spec/models/concerns/after_commit_queue_spec.rb similarity index 94% rename from spec/lib/after_commit_queue_spec.rb rename to spec/models/concerns/after_commit_queue_spec.rb index ca383808bfc..0a35ba34dce 100644 --- a/spec/lib/after_commit_queue_spec.rb +++ b/spec/models/concerns/after_commit_queue_spec.rb @@ -10,7 +10,7 @@ RSpec.describe AfterCommitQueue do project = build(:project) project.run_after_commit(&test_proc) - project.save + project.save! expect(called).to be true end diff --git a/spec/requests/api/graphql/ci/pipelines_spec.rb b/spec/requests/api/graphql/ci/pipelines_spec.rb index 1f47f678898..95ddd0250e7 100644 --- a/spec/requests/api/graphql/ci/pipelines_spec.rb +++ b/spec/requests/api/graphql/ci/pipelines_spec.rb @@ -79,12 +79,13 @@ RSpec.describe 'Query.project(fullPath).pipelines' do create(:ci_build, pipeline: pipeline, stage_id: other_stage.id, name: 'linux: [baz]') end - it 'is null if the user is a guest' do + it 'is present if the user has guest access' do project.add_guest(user) - post_graphql(query, current_user: user, variables: first_n.with(1)) + post_graphql(query, current_user: user) - expect(graphql_data_at(:project, :pipelines, :nodes)).to contain_exactly a_hash_including('stages' => be_nil) + expect(graphql_data_at(:project, :pipelines, :nodes, :stages, :nodes, :name)) + .to contain_exactly(eq(stage.name), eq(other_stage.name)) end it 'is present if the user has reporter access' do @@ -113,12 +114,13 @@ RSpec.describe 'Query.project(fullPath).pipelines' do wrap_fields(query_graphql_path(query_path, :name)) end - it 'is empty if the user is a guest' do + it 'is present if the user has guest access' do project.add_guest(user) post_graphql(query, current_user: user) - expect(graphql_data_at(:project, :pipelines, :nodes, :stages, :nodes, :groups)).to be_empty + expect(graphql_data_at(:project, :pipelines, :nodes, :stages, :nodes, :groups, :nodes, :name)) + .to contain_exactly('linux', 'linux') end it 'is present if the user has reporter access' do diff --git a/spec/requests/api/graphql/project_query_spec.rb b/spec/requests/api/graphql/project_query_spec.rb index e44a7efb354..310a8e9fa33 100644 --- a/spec/requests/api/graphql/project_query_spec.rb +++ b/spec/requests/api/graphql/project_query_spec.rb @@ -143,6 +143,40 @@ RSpec.describe 'getting project information' do end end + context 'when the user has guest access' do + context 'when the project has public pipelines' do + before do + pipeline = create(:ci_pipeline, project: project) + create(:ci_build, project: project, pipeline: pipeline, name: 'a test job') + project.add_guest(current_user) + end + + it 'shows all jobs' do + query = <<~GQL + query { + project(fullPath: "#{project.full_path}") { + jobs { + nodes { + name + stage { + name + } + } + } + } + } + GQL + + post_graphql(query, current_user: current_user) + + expect(graphql_data_at(:project, :jobs, :nodes)).to contain_exactly({ + 'name' => 'a test job', + 'stage' => { 'name' => 'test' } + }) + end + end + end + context 'when the user does not have access to the project' do it 'returns an empty field' do post_graphql(query, current_user: current_user) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b3ebda019ef..613944379b2 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -239,6 +239,7 @@ RSpec.configure do |config| # is not yet opened at the time that is triggered config.prepend_before do ApplicationRecord.set_open_transactions_baseline + ::Ci::ApplicationRecord.set_open_transactions_baseline end config.append_before do @@ -247,6 +248,7 @@ RSpec.configure do |config| config.append_after do ApplicationRecord.reset_open_transactions_baseline + ::Ci::ApplicationRecord.reset_open_transactions_baseline end config.before do |example|