Merge branch 'remove-merge_requests_ci_pipelines_merge_request_id-fk' into 'master'

Swap FK ci_pipelines.merge_request_id to merge_requests for LFK

See merge request gitlab-org/gitlab!78574

app/models/ci/pipeline.rb

@@ -1163,8 +1163,12 @@ module Ci
     end
 
     def merge_request?
+      if Feature.enabled?(:ci_pipeline_merge_request_presence_check, default_enabled: :yaml)
+        merge_request_id.present? && merge_request
+      else
         merge_request_id.present?
       end
+    end
 
     def external_pull_request?
       external_pull_request_id.present?

config/feature_flags/development/ci_pipeline_merge_request_presence_check.yml

+---
+name: ci_pipeline_merge_request_presence_check
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78574
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/350734
+milestone: '14.8'
+type: development
+group: group::pipeline execution
+default_enabled: true

db/post_migrate/20220119151221_remove_merge_requests_ci_pipelines_merge_request_id_fk.rb

+# frozen_string_literal: true
+
+class RemoveMergeRequestsCiPipelinesMergeRequestIdFk < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    return unless foreign_key_exists?(:ci_pipelines, :merge_requests, name: "fk_a23be95014")
+
+    with_lock_retries do
+      execute('LOCK merge_requests, ci_pipelines IN ACCESS EXCLUSIVE MODE') if transaction_open?
+
+      remove_foreign_key_if_exists(:ci_pipelines, :merge_requests, name: "fk_a23be95014")
+    end
+  end
+
+  def down
+    add_concurrent_foreign_key(:ci_pipelines, :merge_requests, name: "fk_a23be95014", column: :merge_request_id, target_column: :id, on_delete: :cascade)
+  end
+end

db/schema_migrations/20220119151221

+7865f26c43c79681f37ceb6e4fecf6153282856907ddfd8211d6d1d57d1fb7f3
\ No newline at end of file

db/structure.sql

@@ -29657,9 +29657,6 @@ ALTER TABLE ONLY issues
 ALTER TABLE ONLY ci_builds
     ADD CONSTRAINT fk_a2141b1522 FOREIGN KEY (auto_canceled_by_id) REFERENCES ci_pipelines(id) ON DELETE SET NULL;
 
-ALTER TABLE ONLY ci_pipelines
-    ADD CONSTRAINT fk_a23be95014 FOREIGN KEY (merge_request_id) REFERENCES merge_requests(id) ON DELETE CASCADE;
-
 ALTER TABLE ONLY bulk_import_entities
     ADD CONSTRAINT fk_a44ff95be5 FOREIGN KEY (parent_id) REFERENCES bulk_import_entities(id) ON DELETE CASCADE;
 

lib/gitlab/database/gitlab_loose_foreign_keys.yml

@@ -87,6 +87,9 @@ ci_pipelines:
   - table: users
     column: user_id
     on_delete: async_nullify
+  - table: merge_requests
+    column: merge_request_id
+    on_delete: async_delete
 ci_project_mirrors:
   - table: projects
     column: project_id

spec/lib/gitlab/database/no_cross_db_foreign_keys_spec.rb

@@ -25,7 +25,6 @@ RSpec.describe 'cross-database foreign keys' do
       ci_pending_builds.project_id
       ci_pipeline_schedules.owner_id
       ci_pipeline_schedules.project_id
-      ci_pipelines.merge_request_id
       ci_pipelines.project_id
       ci_project_monthly_usages.project_id
       ci_refs.project_id

spec/models/ci/pipeline_spec.rb

@@ -390,20 +390,63 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
   end
 
   describe '#merge_request?' do
-    let(:pipeline) { create(:ci_pipeline, merge_request: merge_request) }
-    let(:merge_request) { create(:merge_request) }
+    let_it_be(:merge_request) { create(:merge_request) }
+    let_it_be_with_reload(:pipeline) do
+      create(:ci_pipeline, project: project, merge_request_id: merge_request.id)
+    end
 
-    it 'returns true' do
+    it { expect(pipeline).to be_merge_request }
+
+    context 'when merge request is already loaded' do
+      it 'does not reload the record and returns true' do
+        expect(pipeline.merge_request).to be_present
+
+        expect { pipeline.merge_request? }.not_to exceed_query_limit(0)
         expect(pipeline).to be_merge_request
       end
+    end
 
-    context 'when merge request is nil' do
-      let(:merge_request) { nil }
+    context 'when merge request is not loaded' do
+      it 'executes a database query and returns true' do
+        expect(pipeline).to be_present
 
-      it 'returns false' do
+        expect { pipeline.merge_request? }.not_to exceed_query_limit(1)
+        expect(pipeline).to be_merge_request
+      end
+
+      it 'caches the result' do
+        expect(pipeline).to be_merge_request
+        expect { pipeline.merge_request? }.not_to exceed_query_limit(0)
+      end
+    end
+
+    context 'when merge request was removed' do
+      before do
+        pipeline.update!(merge_request_id: non_existing_record_id)
+      end
+
+      it 'executes a database query and returns false' do
+        expect { pipeline.merge_request? }.not_to exceed_query_limit(1)
         expect(pipeline).not_to be_merge_request
       end
     end
+
+    context 'when merge request id is not present' do
+      before do
+        pipeline.update!(merge_request_id: nil)
+      end
+
+      it { expect(pipeline).not_to be_merge_request }
+    end
+
+    context 'when the feature flag is disabled' do
+      before do
+        stub_feature_flags(ci_pipeline_merge_request_presence_check: false)
+        pipeline.update!(merge_request_id: non_existing_record_id)
+      end
+
+      it { expect(pipeline).to be_merge_request }
+    end
   end
 
   describe '#detached_merge_request_pipeline?' do
@@ -4656,10 +4699,12 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
     let(:factory_name) { :ci_pipeline }
   end
 
+  context 'loose foreign key on ci_pipelines.user_id' do
     it_behaves_like 'cleanup by a loose foreign key' do
       let!(:model) { create(:ci_pipeline, user: create(:user)) }
       let!(:parent) { model.user }
     end
+  end
 
   describe 'tags count' do
     let_it_be_with_refind(:pipeline) do
@@ -4679,4 +4724,11 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
       it { expect(pipeline.distinct_tags_count).to eq(3) }
     end
   end
+
+  context 'loose foreign key on ci_pipelines.merge_request_id' do
+    it_behaves_like 'cleanup by a loose foreign key' do
+      let!(:parent) { create(:merge_request) }
+      let!(:model) { create(:ci_pipeline, merge_request: parent) }
+    end
+  end
 end