Add latest changes from gitlab-org/gitlab@master

.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'

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)
 
         {

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

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_deployment, -> { visible.order(id: :desc) }, 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 :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
 

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

db/schema_migrations/20220802132158

@@ -0,0 +1 @@
+5aeb871227aa1a4c6c08c0e394d7b6324fe55ff6513e83668cf413c569b0b30f

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);

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
 

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

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

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!(:deployment) do
-      let(:pipeline_a) { create(:ci_pipeline, project: project) }
+      create(:deployment, :success, project: project, environment: environment, deployable: deployable)
-      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
     end
 
-    context 'call after preload' do
+    let!(:deployable) { create(:ci_build, :success, project: project) }
-      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
+    it 'fetches the deployable through the last visible deployment' do
-        ActiveRecord::Associations::Preloader.new.preload(environment, [last_visible_deployable: []])
+      is_expected.to eq(deployable)
-
-        query_count = ActiveRecord::QueryRecorder.new do
-          expect(subject.id).to eq(ci_build.id)
-        end.count
-
-        expect(query_count).to eq(0)
-      end
     end
   end
 
   describe '#last_visible_pipeline' do
-    let(:user) { create(:user) }
+    subject { environment.last_visible_pipeline }
-    let_it_be(:project) { create(:project, :repository) }
 
-    let(:environment) { create(:environment, project: project) }
+    let!(:deployment) do
-    let(:commit) { project.commit }
+      create(:deployment, :success, project: project, environment: environment, deployable: deployable)
-
-    let(:success_pipeline) do
-      create(:ci_pipeline, :success, project: project, user: user, sha: commit.sha)
     end
 
-    let(:failed_pipeline) do
+    let!(:deployable) { create(:ci_build, :success, project: project, pipeline: pipeline) }
-      create(:ci_pipeline, :failed, project: project, user: user, sha: commit.sha)
+    let!(:pipeline) { create(:ci_pipeline, :success, project: project) }
-    end
 
-    it 'uses the last deployment even if it failed' do
+    it 'fetches the pipeline through the last visible deployment' do
-      pipeline = create(:ci_pipeline, project: project, user: user, sha: commit.sha)
+      is_expected.to eq(pipeline)
-      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
     end
   end
 

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')

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