Merge branch...

Merge branch '338006-fix-release-features-tables-cross-joining-ci_builds-environments-dashboards-environment' into 'master'

Resolve Environment dashboard features cross joining with CI DB

See merge request gitlab-org/gitlab!68870

app/models/environment.rb

@@ -27,11 +27,10 @@ class Environment < ApplicationRecord
   has_many :alert_management_alerts, class_name: 'AlertManagement::Alert', inverse_of: :environment
 
   has_one :last_deployment, -> { success.distinct_on_environment }, class_name: 'Deployment', inverse_of: :environment
-  has_one :last_deployable, through: :last_deployment, source: 'deployable', source_type: 'CommitStatus'
-  has_one :last_pipeline, through: :last_deployable, source: 'pipeline'
   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'
-  has_one :last_visible_pipeline, through: :last_visible_deployable, source: 'pipeline'
+  has_one :last_visible_deployable, through: :last_visible_deployment, source: 'deployable', source_type: 'CommitStatus', disable_joins: -> { ::Feature.enabled?(:environment_last_visible_pipeline_disable_joins, default_enabled: :yaml) }
+  has_one :last_visible_pipeline, through: :last_visible_deployable, source: 'pipeline', disable_joins: -> { ::Feature.enabled?(:environment_last_visible_pipeline_disable_joins, default_enabled: :yaml) }
+
   has_one :upcoming_deployment, -> { running.distinct_on_environment }, 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
 
@@ -182,6 +181,35 @@ class Environment < ApplicationRecord
     end
   end
 
+  def last_deployable
+    last_deployment&.deployable
+  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) || ::Feature.disabled?(:environment_last_visible_pipeline_disable_joins, default_enabled: :yaml)
+
+    last_visible_deployment&.deployable
+  end
+
+  # Overriding association
+  def last_visible_pipeline
+    return super if association_cached?(:last_visible_pipeline) || ::Feature.disabled?(:environment_last_visible_pipeline_disable_joins, default_enabled: :yaml)
+
+    last_visible_deployable&.pipeline
+  end
+
   def clear_prometheus_reactive_cache!(query_name)
     cluster_prometheus_adapter&.clear_prometheus_reactive_cache!(query_name, self)
   end

config/feature_flags/development/environment_last_visible_pipeline_disable_joins.yml

+---
+name: environment_last_visible_pipeline_disable_joins
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68870
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/340283
+milestone: '14.3'
+type: development
+group: group::release
+default_enabled: true

spec/models/clusters/cluster_spec.rb

@@ -912,8 +912,8 @@ RSpec.describe Clusters::Cluster, :use_clean_rails_memory_store_caching do
     subject { cluster.kubernetes_namespace_for(environment, deployable: build) }
 
     let(:environment_name) { 'the-environment-name' }
-    let(:environment) { create(:environment, name: environment_name, project: cluster.project, last_deployable: build) }
-    let(:build) { create(:ci_build, environment: environment_name, project: cluster.project) }
+    let(:environment) { create(:environment, name: environment_name, project: cluster.project) }
+    let(:build) { create(:ci_build, environment: environment, project: cluster.project) }
     let(:cluster) { create(:cluster, :project, managed: managed_cluster) }
     let(:managed_cluster) { true }
     let(:default_namespace) { Gitlab::Kubernetes::DefaultNamespace.new(cluster, project: cluster.project).from_environment_slug(environment.slug) }

spec/models/environment_spec.rb

@@ -691,6 +691,28 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do
     end
   end
 
+  describe '#last_deployable' do
+    subject { environment.last_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 'when called' do
+        with_cross_joins_prevented do
+          expect(subject.id).to eq(ci_build_a.id)
+        end
+      end
+    end
+  end
+
   describe '#last_visible_deployment' do
     subject { environment.last_visible_deployment }
 
@@ -733,6 +755,86 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do
     end
   end
 
+  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
+    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_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
+
+    context 'when the feature for disable_join is disabled' do
+      let(:pipeline) { create(:ci_pipeline, project: project) }
+      let(:ci_build) { create(:ci_build, project: project, pipeline: pipeline) }
+
+      before do
+        stub_feature_flags(environment_last_visible_pipeline_disable_joins: false)
+        create(:deployment, :failed, project: project, environment: environment, deployable: ci_build)
+      end
+
+      context 'for preload' do
+        it 'executes the original association instead of override' do
+          environment.reload
+          ActiveRecord::Associations::Preloader.new.preload(environment, [last_visible_deployable: []])
+
+          expect_any_instance_of(Deployment).not_to receive(: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
+
+      context 'for direct call' do
+        it 'executes the original association instead of override' do
+          expect_any_instance_of(Deployment).not_to receive(:deployable)
+          expect(subject.id).to eq(ci_build.id)
+        end
+      end
+    end
+  end
+
   describe '#last_visible_pipeline' do
     let(:user) { create(:user) }
     let_it_be(:project) { create(:project, :repository) }
@@ -777,6 +879,35 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do
       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)
@@ -815,6 +946,57 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do
         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
+
+    context 'when the feature for disable_join is disabled' do
+      let(:pipeline) { create(:ci_pipeline, project: project) }
+      let(:ci_build) { create(:ci_build, project: project, pipeline: pipeline) }
+
+      before do
+        stub_feature_flags(environment_last_visible_pipeline_disable_joins: false)
+        create(:deployment, :failed, project: project, environment: environment, deployable: ci_build)
+      end
+
+      subject { environment.last_visible_pipeline }
+
+      context 'for preload' do
+        it 'executes the original association instead of override' do
+          environment.reload
+          ActiveRecord::Associations::Preloader.new.preload(environment, [last_visible_pipeline: []])
+
+          expect_any_instance_of(Ci::Build).not_to receive(:pipeline)
+
+          query_count = ActiveRecord::QueryRecorder.new do
+            expect(subject.id).to eq(pipeline.id)
+          end.count
+
+          expect(query_count).to eq(0)
+        end
+      end
+
+      context 'for direct call' do
+        it 'executes the original association instead of override' do
+          expect_any_instance_of(Ci::Build).not_to receive(:pipeline)
+          expect(subject.id).to eq(pipeline.id)
+        end
+      end
+    end
   end
 
   describe '#upcoming_deployment' do