diff --git a/.rubocop_todo/layout/hash_alignment.yml b/.rubocop_todo/layout/hash_alignment.yml index 003afae5e76..a612b545ac9 100644 --- a/.rubocop_todo/layout/hash_alignment.yml +++ b/.rubocop_todo/layout/hash_alignment.yml @@ -39,38 +39,6 @@ Layout/HashAlignment: - 'app/models/user_status.rb' - 'app/models/wiki.rb' - 'app/models/work_items/type.rb' - - 'ee/app/graphql/mutations/iterations/cadences/create.rb' - - 'ee/app/graphql/mutations/iterations/cadences/destroy.rb' - - 'ee/app/graphql/mutations/iterations/cadences/update.rb' - - 'ee/app/graphql/mutations/iterations/delete.rb' - - 'ee/app/graphql/mutations/projects/set_locked.rb' - - 'ee/app/graphql/resolvers/iterations/cadences_resolver.rb' - - 'ee/app/graphql/resolvers/vulnerabilities_count_per_day_resolver.rb' - - 'ee/app/graphql/types/vulnerabilities/asset_type.rb' - - 'ee/app/graphql/types/vulnerabilities/link_type.rb' - - 'ee/app/graphql/types/vulnerability/external_issue_link_type.rb' - - 'ee/app/graphql/types/vulnerability/issue_link_type.rb' - - 'ee/app/graphql/types/vulnerability_details/base_type.rb' - - 'ee/app/graphql/types/vulnerability_details/boolean_type.rb' - - 'ee/app/graphql/types/vulnerability_details/code_type.rb' - - 'ee/app/graphql/types/vulnerability_details/commit_type.rb' - - 'ee/app/graphql/types/vulnerability_details/diff_type.rb' - - 'ee/app/graphql/types/vulnerability_details/file_location_type.rb' - - 'ee/app/graphql/types/vulnerability_details/int_type.rb' - - 'ee/app/graphql/types/vulnerability_details/list_type.rb' - - 'ee/app/graphql/types/vulnerability_details/markdown_type.rb' - - 'ee/app/graphql/types/vulnerability_details/module_location_type.rb' - - 'ee/app/graphql/types/vulnerability_details/table_type.rb' - - 'ee/app/graphql/types/vulnerability_details/text_type.rb' - - 'ee/app/graphql/types/vulnerability_details/url_type.rb' - - 'ee/app/graphql/types/vulnerability_location/cluster_image_scanning_type.rb' - - 'ee/app/graphql/types/vulnerability_location/container_scanning_type.rb' - - 'ee/app/graphql/types/vulnerability_location/coverage_fuzzing_type.rb' - - 'ee/app/graphql/types/vulnerability_location/dast_type.rb' - - 'ee/app/graphql/types/vulnerability_location/dependency_scanning_type.rb' - - 'ee/app/graphql/types/vulnerability_location/generic_type.rb' - - 'ee/app/graphql/types/vulnerability_location/sast_type.rb' - - 'ee/app/graphql/types/vulnerability_location/secret_detection_type.rb' - 'ee/app/graphql/types/vulnerability_request_response_header_type.rb' - 'ee/app/models/allowed_email_domain.rb' - 'ee/app/models/ci/minutes/usage.rb' diff --git a/app/graphql/mutations/container_repositories/destroy.rb b/app/graphql/mutations/container_repositories/destroy.rb index 1d8f7b22f88..2a45291be22 100644 --- a/app/graphql/mutations/container_repositories/destroy.rb +++ b/app/graphql/mutations/container_repositories/destroy.rb @@ -21,7 +21,9 @@ module Mutations container_repository = authorized_find!(id: id) container_repository.delete_scheduled! + # rubocop:disable CodeReuse/Worker DeleteContainerRepositoryWorker.perform_async(current_user.id, container_repository.id) + # rubocop:enable CodeReuse/Worker track_event(:delete_repository, :container) { diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 68aadf577a0..a3213a59bed 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -206,11 +206,6 @@ class Deployment < ApplicationRecord end end - def self.distinct_on_environment - order('environment_id, deployments.id DESC') - .select('DISTINCT ON (environment_id) deployments.*') - end - def self.find_successful_deployment!(iid) success.find_by!(iid: iid) end diff --git a/app/models/environment.rb b/app/models/environment.rb index 02bdbd86cd1..1950431446b 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -26,12 +26,11 @@ class Environment < ApplicationRecord has_many :self_managed_prometheus_alert_events, inverse_of: :environment has_many :alert_management_alerts, class_name: 'AlertManagement::Alert', inverse_of: :environment + # NOTE: If you preload multiple last deployments of environments, use Preloaders::Environments::DeploymentPreloader. has_one :last_deployment, -> { success.ordered }, class_name: 'Deployment', inverse_of: :environment - has_one :last_visible_deployment, -> { visible.distinct_on_environment }, inverse_of: :environment, class_name: 'Deployment' - has_one :last_visible_deployable, through: :last_visible_deployment, source: 'deployable', source_type: 'CommitStatus', disable_joins: true - has_one :last_visible_pipeline, through: :last_visible_deployable, source: 'pipeline', disable_joins: true + has_one :last_visible_deployment, -> { visible.order(id: :desc) }, inverse_of: :environment, class_name: 'Deployment' - has_one :upcoming_deployment, -> { upcoming.distinct_on_environment }, class_name: 'Deployment', inverse_of: :environment + has_one :upcoming_deployment, -> { upcoming.order(id: :desc) }, class_name: 'Deployment', inverse_of: :environment has_one :latest_opened_most_severe_alert, -> { order_severity_with_open_prometheus_alert }, class_name: 'AlertManagement::Alert', inverse_of: :environment before_validation :generate_slug, if: ->(env) { env.slug.blank? } @@ -216,28 +215,11 @@ class Environment < ApplicationRecord deployable_id: last_deployment_pipeline.latest_builds.pluck(:id)) end - # NOTE: Below assocation overrides is a workaround for issue https://gitlab.com/gitlab-org/gitlab/-/issues/339908 - # It helps to avoid cross joins with the CI database. - # Caveat: It also overrides and losses the default AR caching mechanism. - # Read - https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68870#note_677227727 - - # NOTE: Association Preloads does not use the overriden definitions below. - # Association Preloads when preloading uses the original definitions from the relationships above. - # https://github.com/rails/rails/blob/75ac626c4e21129d8296d4206a1960563cc3d4aa/activerecord/lib/active_record/associations/preloader.rb#L158 - # But after preloading, when they are called it is using the overriden methods below. - # So we are checking for `association_cached?(:association_name)` in the overridden methods and calling `super` which inturn fetches the preloaded values. - - # Overriding association def last_visible_deployable - return super if association_cached?(:last_visible_deployable) - last_visible_deployment&.deployable end - # Overriding association def last_visible_pipeline - return super if association_cached?(:last_visible_pipeline) - last_visible_deployable&.pipeline end diff --git a/db/post_migrate/20220802132158_index_on_integration_type_new_id_when_active_and_has_group.rb b/db/post_migrate/20220802132158_index_on_integration_type_new_id_when_active_and_has_group.rb new file mode 100644 index 00000000000..81e95f531c1 --- /dev/null +++ b/db/post_migrate/20220802132158_index_on_integration_type_new_id_when_active_and_has_group.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class IndexOnIntegrationTypeNewIdWhenActiveAndHasGroup < Gitlab::Database::Migration[2.0] + INDEX_NAME = 'index_integrations_on_type_new_id_when_active_and_has_group' + + disable_ddl_transaction! + + def up + add_concurrent_index :integrations, + [:type_new, :id, :inherit_from_id], + name: INDEX_NAME, + where: '((active = true) AND (group_id IS NOT NULL))' + end + + def down + remove_concurrent_index_by_name :integrations, INDEX_NAME + end +end diff --git a/db/schema_migrations/20220802132158 b/db/schema_migrations/20220802132158 new file mode 100644 index 00000000000..06d7e747f30 --- /dev/null +++ b/db/schema_migrations/20220802132158 @@ -0,0 +1 @@ +5aeb871227aa1a4c6c08c0e394d7b6324fe55ff6513e83668cf413c569b0b30f \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 52fe23f0c45..b0afd780e30 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -28525,6 +28525,8 @@ CREATE INDEX index_integrations_on_type_new ON integrations USING btree (type_ne CREATE INDEX index_integrations_on_type_new_and_instance_partial ON integrations USING btree (type_new, instance) WHERE (instance = true); +CREATE INDEX index_integrations_on_type_new_id_when_active_and_has_group ON integrations USING btree (type_new, id, inherit_from_id) WHERE ((active = true) AND (group_id IS NOT NULL)); + CREATE INDEX index_integrations_on_type_new_id_when_active_and_has_project ON integrations USING btree (type_new, id) WHERE ((active = true) AND (project_id IS NOT NULL)); CREATE INDEX index_integrations_on_unique_group_id_and_type_new ON integrations USING btree (group_id, type_new); diff --git a/doc/development/reusing_abstractions.md b/doc/development/reusing_abstractions.md index f3eb1ebcc0c..87f123f3a57 100644 --- a/doc/development/reusing_abstractions.md +++ b/doc/development/reusing_abstractions.md @@ -109,7 +109,7 @@ the various abstractions and what they can (not) reuse: | Abstraction | Service classes | Finders | Presenters | Serializers | Model instance method | Model class methods | Active Record | Worker |:-----------------------|:-----------------|:---------|:------------|:--------------|:------------------------|:----------------------|:----------------|:-------- -| Controller | Yes | Yes | Yes | Yes | Yes | No | No | No +| Controller/API endpoint| Yes | Yes | Yes | Yes | Yes | No | No | No | Service class | Yes | Yes | No | No | Yes | No | No | Yes | Finder | No | No | No | No | Yes | Yes | No | No | Presenter | No | Yes | No | No | Yes | Yes | No | No @@ -125,9 +125,11 @@ Everything in `app/controllers`. Controllers should not do much work on their own, instead they simply pass input to other classes and present the results. -### Grape endpoint +### API endpoints -Everything in `lib/api`. +Everything in `lib/api` (the REST API) and `app/graphql` (the GraphQL API). + +API endpoints have the same abstraction level as controllers. ### Service classes diff --git a/rubocop/code_reuse_helpers.rb b/rubocop/code_reuse_helpers.rb index 45cfa7ba78d..2769da2389c 100644 --- a/rubocop/code_reuse_helpers.rb +++ b/rubocop/code_reuse_helpers.rb @@ -76,10 +76,15 @@ module RuboCop in_app_directory?(node, 'controllers') end + # Returns true if the given node resides in app/graphql or ee/app/graphql. + def in_graphql?(node) + in_app_directory?(node, 'graphql') + end + # Returns true if the given node resides in app/graphql/types, # ee/app/graphql/types, or ee/app/graphql/ee/types. def in_graphql_types?(node) - in_app_directory?(node, 'graphql/types') || in_app_directory?(node, 'graphql/ee/types') + in_graphql_directory?(node, 'types') end # Returns true if the given node resides in lib/api or ee/lib/api. @@ -113,6 +118,13 @@ module RuboCop ) end + # Returns true if the given node resides in app/graphql/{directory}, + # ee/app/graphql/{directory}, or ee/app/graphql/ee/{directory}. + def in_graphql_directory?(node, directory) + in_app_directory?(node, "graphql/#{directory}") || + in_app_directory?(node, "graphql/ee/#{directory}") + end + # Returns the receiver name of a send node. # # For the AST node `(send (const nil? :Foo) ...)` this would return diff --git a/rubocop/cop/code_reuse/worker.rb b/rubocop/cop/code_reuse/worker.rb index 4902920234f..3a1120ac2a1 100644 --- a/rubocop/cop/code_reuse/worker.rb +++ b/rubocop/cop/code_reuse/worker.rb @@ -10,7 +10,7 @@ module RuboCop include CodeReuseHelpers IN_CONTROLLER = 'Workers can not be used in a controller.' - IN_API = 'Workers can not be used in a Grape API.' + IN_API = 'Workers can not be used in an API endpoint.' IN_FINDER = 'Workers can not be used in a Finder.' IN_PRESENTER = 'Workers can not be used in a Presenter.' IN_SERIALIZER = 'Workers can not be used in a Serializer.' @@ -32,7 +32,7 @@ module RuboCop message = if in_controller?(node) IN_CONTROLLER - elsif in_api?(node) + elsif in_api?(node) || in_graphql?(node) IN_API elsif in_finder?(node) IN_FINDER diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 573e6979924..3f4372dafd0 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -1025,178 +1025,29 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do describe '#last_visible_deployable' do subject { environment.last_visible_deployable } - context 'does not join across databases' do - let(:pipeline_a) { create(:ci_pipeline, project: project) } - let(:pipeline_b) { create(:ci_pipeline, project: project) } - let(:ci_build_a) { create(:ci_build, project: project, pipeline: pipeline_a) } - let(:ci_build_b) { create(:ci_build, project: project, pipeline: pipeline_b) } - - before do - create(:deployment, :success, project: project, environment: environment, deployable: ci_build_a) - create(:deployment, :failed, project: project, environment: environment, deployable: ci_build_b) - end - - it 'for direct call' do - with_cross_joins_prevented do - expect(subject.id).to eq(ci_build_b.id) - end - end - - it 'for preload' do - environment.reload - - with_cross_joins_prevented do - ActiveRecord::Associations::Preloader.new.preload(environment, [last_visible_deployable: []]) - expect(subject.id).to eq(ci_build_b.id) - end - end + let!(:deployment) do + create(:deployment, :success, project: project, environment: environment, deployable: deployable) end - context 'call after preload' do - it 'fetches from association cache' do - pipeline = create(:ci_pipeline, project: project) - ci_build = create(:ci_build, project: project, pipeline: pipeline) - create(:deployment, :failed, project: project, environment: environment, deployable: ci_build) + let!(:deployable) { create(:ci_build, :success, project: project) } - environment.reload - ActiveRecord::Associations::Preloader.new.preload(environment, [last_visible_deployable: []]) - - query_count = ActiveRecord::QueryRecorder.new do - expect(subject.id).to eq(ci_build.id) - end.count - - expect(query_count).to eq(0) - end + it 'fetches the deployable through the last visible deployment' do + is_expected.to eq(deployable) end end describe '#last_visible_pipeline' do - let(:user) { create(:user) } - let_it_be(:project) { create(:project, :repository) } + subject { environment.last_visible_pipeline } - let(:environment) { create(:environment, project: project) } - let(:commit) { project.commit } - - let(:success_pipeline) do - create(:ci_pipeline, :success, project: project, user: user, sha: commit.sha) + let!(:deployment) do + create(:deployment, :success, project: project, environment: environment, deployable: deployable) end - let(:failed_pipeline) do - create(:ci_pipeline, :failed, project: project, user: user, sha: commit.sha) - end + let!(:deployable) { create(:ci_build, :success, project: project, pipeline: pipeline) } + let!(:pipeline) { create(:ci_pipeline, :success, project: project) } - it 'uses the last deployment even if it failed' do - pipeline = create(:ci_pipeline, project: project, user: user, sha: commit.sha) - ci_build = create(:ci_build, project: project, pipeline: pipeline) - create(:deployment, :failed, project: project, environment: environment, deployable: ci_build, sha: commit.sha) - - last_pipeline = environment.last_visible_pipeline - - expect(last_pipeline).to eq(pipeline) - end - - it 'returns nil if there is no deployment' do - create(:ci_build, project: project, pipeline: success_pipeline) - - expect(environment.last_visible_pipeline).to be_nil - end - - it 'does not return an invisible pipeline' do - failed_pipeline = create(:ci_pipeline, project: project, user: user, sha: commit.sha) - ci_build_a = create(:ci_build, project: project, pipeline: failed_pipeline) - create(:deployment, :failed, project: project, environment: environment, deployable: ci_build_a, sha: commit.sha) - pipeline = create(:ci_pipeline, project: project, user: user, sha: commit.sha) - ci_build_b = create(:ci_build, project: project, pipeline: pipeline) - create(:deployment, :created, project: project, environment: environment, deployable: ci_build_b, sha: commit.sha) - - last_pipeline = environment.last_visible_pipeline - - expect(last_pipeline).to eq(failed_pipeline) - end - - context 'does not join across databases' do - let(:pipeline_a) { create(:ci_pipeline, project: project) } - let(:pipeline_b) { create(:ci_pipeline, project: project) } - let(:ci_build_a) { create(:ci_build, project: project, pipeline: pipeline_a) } - let(:ci_build_b) { create(:ci_build, project: project, pipeline: pipeline_b) } - - before do - create(:deployment, :success, project: project, environment: environment, deployable: ci_build_a) - create(:deployment, :failed, project: project, environment: environment, deployable: ci_build_b) - end - - subject { environment.last_visible_pipeline } - - it 'for direct call' do - with_cross_joins_prevented do - expect(subject.id).to eq(pipeline_b.id) - end - end - - it 'for preload' do - environment.reload - - with_cross_joins_prevented do - ActiveRecord::Associations::Preloader.new.preload(environment, [last_visible_pipeline: []]) - expect(subject.id).to eq(pipeline_b.id) - end - end - end - - context 'for the environment' do - it 'returns the last pipeline' do - pipeline = create(:ci_pipeline, project: project, user: user, sha: commit.sha) - ci_build = create(:ci_build, project: project, pipeline: pipeline) - create(:deployment, :success, project: project, environment: environment, deployable: ci_build, sha: commit.sha) - - last_pipeline = environment.last_visible_pipeline - - expect(last_pipeline).to eq(pipeline) - end - - context 'with multiple deployments' do - it 'returns the last pipeline' do - pipeline_a = create(:ci_pipeline, project: project, user: user) - pipeline_b = create(:ci_pipeline, project: project, user: user) - ci_build_a = create(:ci_build, project: project, pipeline: pipeline_a) - ci_build_b = create(:ci_build, project: project, pipeline: pipeline_b) - create(:deployment, :success, project: project, environment: environment, deployable: ci_build_a) - create(:deployment, :success, project: project, environment: environment, deployable: ci_build_b) - - last_pipeline = environment.last_visible_pipeline - - expect(last_pipeline).to eq(pipeline_b) - end - end - - context 'with multiple pipelines' do - it 'returns the last pipeline' do - create(:ci_build, project: project, pipeline: success_pipeline) - ci_build_b = create(:ci_build, project: project, pipeline: failed_pipeline) - create(:deployment, :failed, project: project, environment: environment, deployable: ci_build_b, sha: commit.sha) - - last_pipeline = environment.last_visible_pipeline - - expect(last_pipeline).to eq(failed_pipeline) - end - end - end - - context 'call after preload' do - it 'fetches from association cache' do - pipeline = create(:ci_pipeline, project: project) - ci_build = create(:ci_build, project: project, pipeline: pipeline) - create(:deployment, :failed, project: project, environment: environment, deployable: ci_build) - - environment.reload - ActiveRecord::Associations::Preloader.new.preload(environment, [last_visible_pipeline: []]) - - query_count = ActiveRecord::QueryRecorder.new do - expect(environment.last_visible_pipeline.id).to eq(pipeline.id) - end.count - - expect(query_count).to eq(0) - end + it 'fetches the pipeline through the last visible deployment' do + is_expected.to eq(pipeline) end end diff --git a/spec/rubocop/code_reuse_helpers_spec.rb b/spec/rubocop/code_reuse_helpers_spec.rb index d437ada85ee..0d06d37d67a 100644 --- a/spec/rubocop/code_reuse_helpers_spec.rb +++ b/spec/rubocop/code_reuse_helpers_spec.rb @@ -152,6 +152,26 @@ RSpec.describe RuboCop::CodeReuseHelpers do end end + describe '#in_graphql?' do + it 'returns true for a node in the FOSS GraphQL directory' do + node = build_and_parse_source('10', rails_root_join('app', 'graphql', 'foo.rb')) + + expect(cop.in_graphql?(node)).to eq(true) + end + + it 'returns true for a node in the EE GraphQL directory' do + node = build_and_parse_source('10', rails_root_join('ee', 'app', 'graphql', 'foo.rb')) + + expect(cop.in_graphql?(node)).to eq(true) + end + + it 'returns false for a node outside the GraphQL directory' do + node = build_and_parse_source('10', rails_root_join('app', 'foo', 'foo.rb')) + + expect(cop.in_graphql?(node)).to eq(false) + end + end + describe '#in_graphql_types?' do %w[ app/graphql/types @@ -169,7 +189,7 @@ RSpec.describe RuboCop::CodeReuseHelpers do app/graphql/resolvers app/foo ].each do |path| - it "returns true for a node in #{path}" do + it "returns false for a node in #{path}" do node = build_and_parse_source('10', rails_root_join(path, 'foo.rb')) expect(cop.in_graphql_types?(node)).to eq(false) @@ -255,6 +275,44 @@ RSpec.describe RuboCop::CodeReuseHelpers do end end + describe '#in_graphql_directory?' do + it 'returns true for a directory in the FOSS app/graphql directory' do + node = build_and_parse_source('10', rails_root_join('app', 'graphql', 'subdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(true) + end + + it 'returns true for a directory in the EE app/graphql directory' do + node = build_and_parse_source('10', rails_root_join('ee', 'app', 'graphql', 'subdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(true) + end + + it 'returns true for a directory in the EE app/graphql/ee directory' do + node = build_and_parse_source('10', rails_root_join('ee', 'app', 'graphql', 'ee', 'subdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(true) + end + + it 'returns false for a directory in the FOSS app/graphql directory' do + node = build_and_parse_source('10', rails_root_join('app', 'graphql', 'anotherdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(false) + end + + it 'returns false for a directory in the EE app/graphql directory' do + node = build_and_parse_source('10', rails_root_join('ee', 'app', 'graphql', 'anotherdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(false) + end + + it 'returns false for a directory in the EE app/graphql/ee directory' do + node = build_and_parse_source('10', rails_root_join('ee', 'app', 'graphql', 'ee', 'anotherdir', 'foo.rb')) + + expect(cop.in_graphql_directory?(node, 'subdir')).to eq(false) + end + end + describe '#name_of_receiver' do it 'returns the name of a send receiver' do node = build_and_parse_source('Foo.bar') diff --git a/spec/rubocop/cop/code_reuse/worker_spec.rb b/spec/rubocop/cop/code_reuse/worker_spec.rb index 8155791a3e3..a548e90d8e1 100644 --- a/spec/rubocop/cop/code_reuse/worker_spec.rb +++ b/spec/rubocop/cop/code_reuse/worker_spec.rb @@ -31,7 +31,24 @@ RSpec.describe RuboCop::Cop::CodeReuse::Worker do resource :projects do get '/' do FooWorker.perform_async - ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in a Grape API. + ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in an API endpoint. + end + end + end + SOURCE + end + + it 'flags the use of a worker in GraphQL' do + allow(cop) + .to receive(:in_graphql?) + .and_return(true) + + expect_offense(<<~SOURCE) + module Mutations + class Foo < BaseMutation + def resolve + FooWorker.perform_async + ^^^^^^^^^^^^^^^^^^^^^^^ Workers can not be used in an API endpoint. end end end