Merge branch 'aw-fix-source-pipeline-source-project' into 'master'

Use root namespace for source pipeline projects

See merge request gitlab-org/gitlab!88045

app/models/ci/pipeline.rb

@@ -81,6 +81,7 @@ class Pipeline < Ci::ApplicationRecord
     has_many :downloadable_artifacts, -> do
       not_expired.or(where_exists(::Ci::Pipeline.artifacts_locked.where('ci_pipelines.id = ci_builds.commit_id'))).downloadable.with_job
     end, through: :latest_builds, source: :job_artifacts
+    has_many :latest_successful_builds, -> { latest.success.with_project_and_metadata }, foreign_key: :commit_id, inverse_of: :pipeline, class_name: 'Ci::Build'
     has_many :messages, class_name: 'Ci::PipelineMessage', inverse_of: :pipeline

app/models/ci/sources/pipeline.rb

@@ -7,10 +7,10 @@ class Pipeline < Ci::ApplicationRecord
       self.table_name = "ci_sources_pipelines"
-      belongs_to :project, class_name: "Project"
+      belongs_to :project, class_name: "::Project"
       belongs_to :pipeline, class_name: "Ci::Pipeline", inverse_of: :source_pipeline
-      belongs_to :source_project, class_name: "Project", foreign_key: :source_project_id
+      belongs_to :source_project, class_name: "::Project", foreign_key: :source_project_id
       belongs_to :source_job, class_name: "CommitStatus", foreign_key: :source_job_id
       belongs_to :source_bridge, class_name: "Ci::Bridge", foreign_key: :source_job_id
       belongs_to :source_pipeline, class_name: "Ci::Pipeline", foreign_key: :source_pipeline_id

app/models/deployment.rb

@@ -182,6 +182,38 @@ def self.last_for_environment(environment)
     find(ids)
   end
+  # This method returns the deployment records of the last deployment pipeline, that successfully executed for the given environment.
+  # e.g.
+  # A pipeline contains
+  #   - deploy job A => production environment
+  #   - deploy job B => production environment
+  # In this case, `last_deployment_group` returns both deployments.
+  #
+  # NOTE: Preload environment.last_deployment and pipeline.latest_successful_builds prior to avoid N+1.
+  def self.last_deployment_group_for_environment(env)
+    return self.none unless env.last_deployment_pipeline&.latest_successful_builds&.present?
+
+    BatchLoader.for(env).batch do |environments, loader|
+      latest_successful_build_ids = []
+      environments_hash = {}
+
+      environments.each do |environment|
+        environments_hash[environment.id] = environment
+
+        # Refer comment note above, if not preloaded this can lead to N+1.
+        latest_successful_build_ids << environment.last_deployment_pipeline.latest_successful_builds.map(&:id)
+      end
+
+      Deployment
+        .where(deployable_type: 'CommitStatus', deployable_id: latest_successful_build_ids.flatten)
+        .preload(last_deployment_group_associations)
+        .group_by { |deployment| deployment.environment_id }
+        .each do |env_id, deployment_group|
+          loader.call(environments_hash[env_id], deployment_group)
+        end
+    end
+  end
+
   def self.distinct_on_environment
     order('environment_id, deployments.id DESC')
       .select('DISTINCT ON (environment_id) deployments.*')
@@ -439,6 +471,18 @@ def update_status!(status)
       raise ArgumentError, "The status #{status.inspect} is invalid"
     end
   end
+
+  def self.last_deployment_group_associations
+    {
+      deployable: {
+        pipeline: {
+          manual_actions: []
+        }
+      }
+    }
+  end
+
+  private_class_method :last_deployment_group_associations
 end
 Deployment.prepend_mod_with('Deployment')

app/models/environment.rb

@@ -202,7 +202,7 @@ def last_deployment_pipeline
   #   - deploy job A => production environment
   #   - deploy job B => production environment
   # In this case, `last_deployment_group` returns both deployments, whereas `last_deployable` returns only B.
-  def last_deployment_group
+  def legacy_last_deployment_group
     return Deployment.none unless last_deployment_pipeline
     successful_deployments.where(
@@ -314,12 +314,18 @@ def stop_with_actions!(current_user)
   def stop_actions
     strong_memoize(:stop_actions) do
-      # Fix N+1 queries it brings to the serializer.
-      # Tracked in https://gitlab.com/gitlab-org/gitlab/-/issues/358780
       last_deployment_group.map(&:stop_action).compact
     end
   end
+  def last_deployment_group
+    if ::Feature.enabled?(:batch_load_environment_last_deployment_group, project)
+      Deployment.last_deployment_group_for_environment(self)
+    else
+      legacy_last_deployment_group
+    end
+  end
+
   def reset_auto_stop
     update_column(:auto_stop_at, nil)
   end

app/serializers/environment_serializer.rb

@@ -52,19 +52,30 @@ def itemize(resource)
   end
   def batch_load(resource)
+    temp_deployment_associations = deployment_associations
+
     resource = resource.preload(environment_associations.except(:last_deployment, :upcoming_deployment))
+    if ::Feature.enabled?(:batch_load_environment_last_deployment_group, resource.first&.project)
+      temp_deployment_associations[:deployable][:pipeline][:latest_successful_builds] = []
+    end
+
     Preloaders::Environments::DeploymentPreloader.new(resource)
-      .execute_with_union(:last_deployment, deployment_associations)
+      .execute_with_union(:last_deployment, temp_deployment_associations)
     Preloaders::Environments::DeploymentPreloader.new(resource)
-      .execute_with_union(:upcoming_deployment, deployment_associations)
+      .execute_with_union(:upcoming_deployment, temp_deployment_associations)
     resource.to_a.tap do |environments|
       environments.each do |environment|
         # Batch loading the commits of the deployments
         environment.last_deployment&.commit&.try(:lazy_author)
         environment.upcoming_deployment&.commit&.try(:lazy_author)
+
+        if ::Feature.enabled?(:batch_load_environment_last_deployment_group, environment.project)
+          # Batch loading last_deployment_group which is called later by environment.stop_actions
+          environment.last_deployment_group
+        end
       end
     end
   end
@@ -89,10 +100,11 @@ def deployment_associations
         user: [],
         metadata: [],
         pipeline: {
-          manual_actions: [:metadata],
+          manual_actions: [:metadata, :deployment],
           scheduled_actions: [:metadata]
         },
-        project: project_associations
+        project: project_associations,
+        deployment: []
       }
     }
   end

config/feature_flags/development/batch_load_environment_last_deployment_group.yml

+---
+name: batch_load_environment_last_deployment_group
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/86584/
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/363023
+milestone: '15.1'
+type: development
+group: group::release
+default_enabled: false

spec/features/projects/environments/environment_spec.rb

@@ -261,6 +261,8 @@ def auto_stop_button_selector
           context 'when environment is available' do
             context 'with stop action' do
+              let(:build) { create(:ci_build, :success, pipeline: pipeline, environment: environment.name) }
+
               let(:action) do
                 create(:ci_build, :manual, pipeline: pipeline,
                        name: 'close_app',

spec/lib/gitlab/import_export/all_models.yml

@@ -274,6 +274,7 @@ ci_pipelines:
 - security_findings
 - daily_build_group_report_results
 - latest_builds
+- latest_successful_builds
 - daily_report_results
 - latest_builds_report_results
 - messages

spec/models/ci/pipeline_spec.rb

@@ -73,6 +73,17 @@
       end
     end
+    describe '#latest_successful_builds' do
+      it 'has a one to many relationship with its latest successful builds' do
+        _old_build = create(:ci_build, :retried, pipeline: pipeline)
+        _expired_build = create(:ci_build, :expired, pipeline: pipeline)
+        _failed_builds = create_list(:ci_build, 2, :failed, pipeline: pipeline)
+        successful_builds = create_list(:ci_build, 2, :success, pipeline: pipeline)
+
+        expect(pipeline.latest_successful_builds).to contain_exactly(successful_builds.first, successful_builds.second)
+      end
+    end
+
     describe '#downloadable_artifacts' do
       let_it_be(:build) { create(:ci_build, pipeline: pipeline) }
       let_it_be(:downloadable_artifact) { create(:ci_job_artifact, :codequality, job: build) }

spec/models/ci/sources/pipeline_spec.rb

@@ -6,7 +6,7 @@
   it { is_expected.to belong_to(:project) }
   it { is_expected.to belong_to(:pipeline) }
-  it { is_expected.to belong_to(:source_project) }
+  it { is_expected.to belong_to(:source_project).class_name('::Project') }
   it { is_expected.to belong_to(:source_job) }
   it { is_expected.to belong_to(:source_bridge) }
   it { is_expected.to belong_to(:source_pipeline) }

spec/models/deployment_spec.rb

@@ -640,6 +640,143 @@
         is_expected.to contain_exactly(deployment1, deployment2)
       end
     end
+
+    describe 'last_deployment_group_for_environment' do
+      def subject_method(environment)
+        described_class.last_deployment_group_for_environment(environment)
+      end
+
+      let!(:project) { create(:project, :repository) }
+      let!(:environment) { create(:environment, project: project) }
+
+      context 'when there are no deployments and builds' do
+        it do
+          expect(subject_method(environment)).to eq(Deployment.none)
+        end
+      end
+
+      context 'when there are no successful builds' do
+        let(:pipeline) { create(:ci_pipeline, project: project) }
+        let(:ci_build) { create(:ci_build, :running, project: project, pipeline: pipeline) }
+
+        before do
+          create(:deployment, :success, environment: environment, project: project, deployable: ci_build)
+        end
+
+        it do
+          expect(subject_method(environment)).to eq(Deployment.none)
+        end
+      end
+
+      context 'when there are deployments for multiple pipelines' do
+        let(:pipeline_a) { create(:ci_pipeline, project: project) }
+        let(:pipeline_b) { create(:ci_pipeline, project: project) }
+        let(:ci_build_a) { create(:ci_build, :success, project: project, pipeline: pipeline_a) }
+        let(:ci_build_b) { create(:ci_build, :failed, project: project, pipeline: pipeline_b) }
+        let(:ci_build_c) { create(:ci_build, :success, project: project, pipeline: pipeline_a) }
+        let(:ci_build_d) { create(:ci_build, :failed, project: project, pipeline: pipeline_a) }
+
+        # Successful deployments for pipeline_a
+        let!(:deployment_a) do
+          create(:deployment, :success, project: project, environment: environment, deployable: ci_build_a)
+        end
+
+        let!(:deployment_b) do
+          create(:deployment, :success, project: project, environment: environment, deployable: ci_build_c)
+        end
+
+        before do
+          # Failed deployment for pipeline_a
+          create(:deployment, :failed, project: project, environment: environment, deployable: ci_build_d)
+
+          # Failed deployment for pipeline_b
+          create(:deployment, :failed, project: project, environment: environment, deployable: ci_build_b)
+        end
+
+        it 'returns the successful deployment jobs for the last deployment pipeline' do
+          expect(subject_method(environment).pluck(:id)).to contain_exactly(deployment_a.id, deployment_b.id)
+        end
+      end
+
+      context 'when there are many environments' do
+        let(:environment_b) { create(:environment, project: project) }
+
+        let(:pipeline_a) { create(:ci_pipeline, project: project) }
+        let(:pipeline_b) { create(:ci_pipeline, project: project) }
+        let(:pipeline_c) { create(:ci_pipeline, project: project) }
+        let(:pipeline_d) { create(:ci_pipeline, project: project) }
+
+        # Builds for first environment: 'environment' with pipeline_a and pipeline_b
+        let(:ci_build_a) { create(:ci_build, :success, project: project, pipeline: pipeline_a) }
+        let(:ci_build_b) { create(:ci_build, :failed, project: project, pipeline: pipeline_b) }
+        let(:ci_build_c) { create(:ci_build, :success, project: project, pipeline: pipeline_a) }
+        let(:ci_build_d) { create(:ci_build, :failed, project: project, pipeline: pipeline_a) }
+        let!(:stop_env_a) { create(:ci_build, :manual, project: project, pipeline: pipeline_a, name: 'stop_env_a') }
+
+        # Builds for second environment: 'environment_b' with pipeline_c and pipeline_d
+        let(:ci_build_e) { create(:ci_build, :success, project: project, pipeline: pipeline_c) }
+        let(:ci_build_f) { create(:ci_build, :failed, project: project, pipeline: pipeline_d) }
+        let(:ci_build_g) { create(:ci_build, :success, project: project, pipeline: pipeline_c) }
+        let(:ci_build_h) { create(:ci_build, :failed, project: project, pipeline: pipeline_c) }
+        let!(:stop_env_b) { create(:ci_build, :manual, project: project, pipeline: pipeline_c, name: 'stop_env_b') }
+
+        # Successful deployments for 'environment' from pipeline_a
+        let!(:deployment_a) do
+          create(:deployment, :success, project: project, environment: environment, deployable: ci_build_a)
+        end
+
+        let!(:deployment_b) do
+          create(:deployment, :success,
+            project: project, environment: environment, deployable: ci_build_c, on_stop: 'stop_env_a')
+        end
+
+        # Successful deployments for 'environment_b' from pipeline_c
+        let!(:deployment_c) do
+          create(:deployment, :success, project: project, environment: environment_b, deployable: ci_build_e)
+        end
+
+        let!(:deployment_d) do
+          create(:deployment, :success,
+            project: project, environment: environment_b, deployable: ci_build_g, on_stop: 'stop_env_b')
+        end
+
+        before do
+          # Failed deployment for 'environment' from pipeline_a and pipeline_b
+          create(:deployment, :failed, project: project, environment: environment, deployable: ci_build_d)
+          create(:deployment, :failed, project: project, environment: environment, deployable: ci_build_b)
+
+          # Failed deployment for 'environment_b' from pipeline_c and pipeline_d
+          create(:deployment, :failed, project: project, environment: environment_b, deployable: ci_build_h)
+          create(:deployment, :failed, project: project, environment: environment_b, deployable: ci_build_f)
+        end
+
+        it 'batch loads for environments' do
+          environments = [environment, environment_b]
+
+          # Loads Batch loader
+          environments.each do |env|
+            subject_method(env)
+          end
+
+          expect(subject_method(environments.first).pluck(:id))
+            .to contain_exactly(deployment_a.id, deployment_b.id)
+
+          expect { subject_method(environments.second).pluck(:id) }.not_to exceed_query_limit(0)
+
+          expect(subject_method(environments.second).pluck(:id))
+            .to contain_exactly(deployment_c.id, deployment_d.id)
+
+          expect(subject_method(environments.first).map(&:stop_action).compact)
+            .to contain_exactly(stop_env_a)
+
+          expect { subject_method(environments.second).map(&:stop_action) }
+            .not_to exceed_query_limit(0)
+
+          expect(subject_method(environments.second).map(&:stop_action).compact)
+            .to contain_exactly(stop_env_b)
+        end
+      end
+    end
   end
   describe 'latest_for_sha' do

spec/models/environment_spec.rb

@@ -479,7 +479,7 @@
     end
     context 'when matching action is defined' do
-      let(:build) { create(:ci_build) }
+      let(:build) { create(:ci_build, :success) }
       let!(:deployment) do
         create(:deployment, :success,
@@ -549,7 +549,7 @@
     context 'when matching action is defined' do
       let(:pipeline) { create(:ci_pipeline, project: project) }
-      let(:build_a) { create(:ci_build, pipeline: pipeline) }
+      let(:build_a) { create(:ci_build, :success, pipeline: pipeline) }
       before do
         create(:deployment, :success,
@@ -576,6 +576,7 @@
                  name: 'master', project: project)
         end
+        # check this test if this makes sense.
         context 'when action did not yet finish' do
           let!(:close_action) do
             create(:ci_build, :manual, pipeline: pipeline, name: 'close_app_a')
@@ -632,8 +633,8 @@
     context 'when there are more then one stop action for the environment' do
       let(:pipeline) { create(:ci_pipeline, project: project) }
-      let(:build_a) { create(:ci_build, pipeline: pipeline) }
-      let(:build_b) { create(:ci_build, pipeline: pipeline) }
+      let(:build_a) { create(:ci_build, :success, pipeline: pipeline) }
+      let(:build_b) { create(:ci_build, :success, pipeline: pipeline) }
       let!(:close_actions) do
         [
@@ -666,9 +667,9 @@
         expect(actions.pluck(:user)).to match_array(close_actions.pluck(:user))
       end
-      context 'when there are failed deployment jobs' do
+      context 'when there are failed builds' do
         before do
-          create(:ci_build, pipeline: pipeline, name: 'close_app_c')
+          create(:ci_build, :failed, pipeline: pipeline, name: 'close_app_c')
           create(:deployment, :failed,
             environment: environment,
@@ -676,11 +677,11 @@
             on_stop: 'close_app_c')
         end
-        it 'returns only stop actions from successful deployment jobs' do
+        it 'returns only stop actions from successful builds' do
           actions = subject
           expect(actions).to match_array(close_actions)
-          expect(actions.count).to eq(environment.successful_deployments.count)
+          expect(actions.count).to eq(pipeline.latest_successful_builds.count)
         end
       end
     end
@@ -697,8 +698,8 @@
     context 'when there are multiple deployments with actions' do
       let(:pipeline) { create(:ci_pipeline, project: project) }
-      let(:ci_build_a) { create(:ci_build, project: project, pipeline: pipeline) }
-      let(:ci_build_b) { create(:ci_build, project: project, pipeline: pipeline) }
+      let(:ci_build_a) { create(:ci_build, :success, project: project, pipeline: pipeline) }
+      let(:ci_build_b) { create(:ci_build, :success, project: project, pipeline: pipeline) }
       let!(:ci_build_c) { create(:ci_build, :manual, project: project, pipeline: pipeline, name: 'close_app_a') }
       let!(:ci_build_d) { create(:ci_build, :manual, project: project, pipeline: pipeline, name: 'close_app_b') }
@@ -714,7 +715,7 @@
       before do
         # Create failed deployment without stop_action.
-        build = create(:ci_build, project: project, pipeline: pipeline)
+        build = create(:ci_build, :failed, project: project, pipeline: pipeline)
         create(:deployment, :failed, project: project, environment: environment, deployable: build)
       end
@@ -736,10 +737,10 @@
     context 'when there are deployments for multiple pipelines' 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) }
-      let(:ci_build_c) { create(:ci_build, project: project, pipeline: pipeline_a) }
-      let(:ci_build_d) { create(:ci_build, project: project, pipeline: pipeline_a) }
+      let(:ci_build_a) { create(:ci_build, :success, project: project, pipeline: pipeline_a) }
+      let(:ci_build_b) { create(:ci_build, :failed, project: project, pipeline: pipeline_b) }
+      let(:ci_build_c) { create(:ci_build, :success, project: project, pipeline: pipeline_a) }
+      let(:ci_build_d) { create(:ci_build, :failed, project: project, pipeline: pipeline_a) }
       # Successful deployments for pipeline_a
       let!(:deployment_a) { create(:deployment, :success, project: project, environment: environment, deployable: ci_build_a) }
@@ -756,6 +757,16 @@
       it 'returns the successful deployment jobs for the last deployment pipeline' do
         expect(subject.pluck(:id)).to contain_exactly(deployment_a.id, deployment_b.id)
       end
+
+      context 'when the feature flag is disabled' do
+        before do
+          stub_feature_flags(batch_load_environment_last_deployment_group: false)
+        end
+
+        it 'returns the successful deployment jobs for the last deployment pipeline' do
+          expect(subject.pluck(:id)).to contain_exactly(deployment_a.id, deployment_b.id)
+        end
+      end
     end
   end

spec/serializers/environment_serializer_spec.rb

@@ -212,7 +212,10 @@
       upcoming_deployment = nil
       create(:environment, project: project).tap do |environment|
         create(:deployment, :success, environment: environment, project: project)
-        last_deployment = create(:deployment, :success, environment: environment, project: project)
+
+        create(:ci_build, :success, project: project).tap do |build|
+          last_deployment = create(:deployment, :success, environment: environment, project: project, deployable: build)
+        end
         create(:deployment, :running, environment: environment, project: project)
         upcoming_deployment = create(:deployment, :running, environment: environment, project: project)
@@ -227,11 +230,22 @@
   def create_environment_with_associations(project)
     create(:environment, project: project).tap do |environment|
-      pipeline = create(:ci_pipeline, project: project)
-      manual_build = create(:ci_build, :manual, project: project, pipeline: pipeline, environment: environment.name)
-      scheduled_build = create(:ci_build, :scheduled, project: project, pipeline: pipeline, environment: environment.name)
-      create(:deployment, :success, environment: environment, project: project, deployable: manual_build)
-      create(:deployment, :running, environment: environment, project: project, deployable: scheduled_build)
+      create(:ci_pipeline, project: project).tap do |pipeline|
+        create(:ci_build, :manual, project: project, pipeline: pipeline, name: 'stop-action',
+           environment: environment.name)
+
+        create(:ci_build, :scheduled, project: project, pipeline: pipeline,
+          environment: environment.name).tap do |scheduled_build|
+            create(:deployment, :running, environment: environment, project: project,
+              deployable: scheduled_build)
+          end
+
+        create(:ci_build, :success, :manual, project: project, pipeline: pipeline,
+          environment: environment.name).tap do |manual_build|
+            create(:deployment, :success, environment: environment, project: project,
+              deployable: manual_build, on_stop: 'stop-action')
+          end
+      end
     end
   end
 end

spec/support/shared_examples/serializers/environment_serializer_shared_examples.rb

@@ -8,9 +8,8 @@
     create_environment_with_associations(project)
     create_environment_with_associations(project)
-    # Fix N+1 queries introduced by multi stop_actions for environment.
-    # Tracked in https://gitlab.com/gitlab-org/gitlab/-/issues/358780
-    relax_count = 14
+    # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/363317
+    relax_count = 1
     expect { serialize(grouping: true) }.not_to exceed_query_limit(control.count + relax_count)
   end
@@ -23,9 +22,8 @@
     create_environment_with_associations(project)
     create_environment_with_associations(project)
-    # Fix N+1 queries introduced by multi stop_actions for environment.
-    # Tracked in https://gitlab.com/gitlab-org/gitlab/-/issues/358780
-    relax_count = 14
+    # See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/363317
+    relax_count = 1
     expect { serialize(grouping: false) }.not_to exceed_query_limit(control.count + relax_count)
   end