Merge branch 'avoid-cross-joins-in-environment-model' into 'master'

Resolve "Avoid cross joins in environment model"

See merge request gitlab-org/gitlab!71577

app/models/deployment.rb

@@ -46,7 +46,6 @@ class Deployment < ApplicationRecord
   scope :stoppable, -> { where.not(on_stop: nil).where.not(deployable_id: nil).success }
   scope :active, -> { where(status: %i[created running]) }
   scope :older_than, -> (deployment) { where('deployments.id < ?', deployment.id) }
-  scope :with_deployable, -> { joins('INNER JOIN ci_builds ON ci_builds.id = deployments.deployable_id').preload(:deployable) }
   scope :with_api_entity_associations, -> { preload({ deployable: { runner: [], tags: [], user: [], job_artifacts_archive: [] } }) }
 
   scope :finished_after, ->(date) { where('finished_at >= ?', date) }
@@ -148,6 +147,16 @@ class Deployment < ApplicationRecord
     success.find_by!(iid: iid)
   end
 
+  # It should be used with caution especially on chaining.
+  # Fetching any unbounded or large intermediate dataset could lead to loading too many IDs into memory.
+  # See: https://docs.gitlab.com/ee/development/database/multiple_databases.html#use-disable_joins-for-has_one-or-has_many-through-relations
+  # For safety we default limit to fetch not more than 1000 records.
+  def self.builds(limit = 1000)
+    deployable_ids = where.not(deployable_id: nil).limit(limit).pluck(:deployable_id)
+
+    Ci::Build.where(id: deployable_ids)
+  end
+
   class << self
     ##
     # FastDestroyAll concerns

app/models/environment.rb

@@ -260,10 +260,9 @@ class Environment < ApplicationRecord
   end
 
   def cancel_deployment_jobs!
-    jobs = active_deployments.with_deployable
-    jobs.each do |deployment|
-      Gitlab::OptimisticLocking.retry_lock(deployment.deployable, name: 'environment_cancel_deployment_jobs') do |deployable|
-        deployable.cancel! if deployable&.cancelable?
+    active_deployments.builds.each do |build|
+      Gitlab::OptimisticLocking.retry_lock(build, name: 'environment_cancel_deployment_jobs') do |build|
+        build.cancel! if build&.cancelable?
       end
     rescue StandardError => e
       Gitlab::ErrorTracking.track_exception(e, environment_id: id, deployment_id: deployment.id)

app/services/deployments/older_deployments_drop_service.rb

@@ -11,23 +11,23 @@ module Deployments
     def execute
       return unless @deployment&.running?
 
-      older_deployments.find_each do |older_deployment|
-        Gitlab::OptimisticLocking.retry_lock(older_deployment.deployable, name: 'older_deployments_drop') do |deployable|
-          deployable.drop(:forward_deployment_failure)
+      older_deployments_builds.each do |build|
+        Gitlab::OptimisticLocking.retry_lock(build, name: 'older_deployments_drop') do |build|
+          build.drop(:forward_deployment_failure)
         end
       rescue StandardError => e
-        Gitlab::ErrorTracking.track_exception(e, subject_id: @deployment.id, deployment_id: older_deployment.id)
+        Gitlab::ErrorTracking.track_exception(e, subject_id: @deployment.id, build_id: build.id)
       end
     end
 
     private
 
-    def older_deployments
+    def older_deployments_builds
       @deployment
         .environment
         .active_deployments
         .older_than(@deployment)
-        .with_deployable
+        .builds
     end
   end
 end

ee/app/services/deployments/auto_rollback_service.rb

@@ -39,10 +39,10 @@ module Deployments
       return unless previous_commit_ids
 
       rollback_target = environment.successful_deployments
-        .with_deployable
         .latest_for_sha(previous_commit_ids)
 
-      return unless rollback_target && rollback_target.deployable.retryable?
+      # Safely navigate deployable because of https://gitlab.com/gitlab-org/gitlab/-/issues/218659
+      return unless rollback_target && rollback_target.deployable&.retryable?
 
       rollback_target
     end

ee/spec/services/deployments/auto_rollback_service_spec.rb

@@ -97,6 +97,16 @@ RSpec.describe Deployments::AutoRollbackService, :clean_gitlab_redis_rate_limiti
       end
     end
 
+    context "when rollback target's deployable is not available" do
+      before do
+        environment.all_deployments.first.deployable.destroy!
+      end
+
+      it_behaves_like 'rollback failure' do
+        let(:message) { 'Failed to find a rollback target.' }
+      end
+    end
+
     context "when rollback target's deployable is not retryable" do
       before do
         environment.all_deployments.first.deployable.degenerate!

spec/models/deployment_spec.rb

@@ -456,18 +456,6 @@ RSpec.describe Deployment do
       end
     end
 
-    describe 'with_deployable' do
-      subject { described_class.with_deployable }
-
-      it 'retrieves deployments with deployable builds' do
-        with_deployable = create(:deployment)
-        create(:deployment, deployable: nil)
-        create(:deployment, deployable_type: 'CommitStatus', deployable_id: non_existing_record_id)
-
-        is_expected.to contain_exactly(with_deployable)
-      end
-    end
-
     describe 'visible' do
       subject { described_class.visible }
 
@@ -613,6 +601,26 @@ RSpec.describe Deployment do
     end
   end
 
+  describe '.builds' do
+    let!(:deployment1) { create(:deployment) }
+    let!(:deployment2) { create(:deployment) }
+    let!(:deployment3) { create(:deployment) }
+
+    subject { described_class.builds }
+
+    it 'retrieves builds for the deployments' do
+      is_expected.to match_array(
+        [deployment1.deployable, deployment2.deployable, deployment3.deployable])
+    end
+
+    it 'does not fetch the null deployable_ids' do
+      deployment3.update!(deployable_id: nil, deployable_type: nil)
+
+      is_expected.to match_array(
+        [deployment1.deployable, deployment2.deployable])
+    end
+  end
+
   describe '#previous_deployment' do
     using RSpec::Parameterized::TableSyntax
 

spec/services/deployments/older_deployments_drop_service_spec.rb

@@ -84,7 +84,7 @@ RSpec.describe Deployments::OlderDeploymentsDropService do
 
             it 'does not drop an older deployment and tracks the exception' do
               expect(Gitlab::ErrorTracking).to receive(:track_exception)
-                .with(kind_of(RuntimeError), subject_id: deployment.id, deployment_id: older_deployment.id)
+                .with(kind_of(RuntimeError), subject_id: deployment.id, build_id: older_deployment.deployable_id)
 
               expect { subject }.not_to change { Ci::Build.failed.count }
             end

spec/support/database/cross-join-allowlist.yml

@@ -70,8 +70,6 @@
 - "./spec/models/ci/job_artifact_spec.rb"
 - "./spec/models/ci/pipeline_spec.rb"
 - "./spec/models/ci/runner_spec.rb"
-- "./spec/models/deployment_spec.rb"
-- "./spec/models/environment_spec.rb"
 - "./spec/models/merge_request_spec.rb"
 - "./spec/models/project_spec.rb"
 - "./spec/models/user_spec.rb"