Merge branch '336891-decompose-queries-in-PipelinesForMergeRequestFinder' into 'master'

Avoid cross-joins in `PipelinesForMergeRequestFinder`

See merge request gitlab-org/gitlab!68549

app/finders/ci/pipelines_for_merge_request_finder.rb

@@ -5,6 +5,8 @@ module Ci
   class PipelinesForMergeRequestFinder
     include Gitlab::Utils::StrongMemoize
 
+    COMMITS_LIMIT = 100
+
     def initialize(merge_request, current_user)
       @merge_request = merge_request
       @current_user = current_user
@@ -12,7 +14,7 @@ module Ci
 
     attr_reader :merge_request, :current_user
 
-    delegate :commit_shas, :target_project, :source_project, :source_branch, to: :merge_request
+    delegate :recent_diff_head_shas, :commit_shas, :target_project, :source_project, :source_branch, to: :merge_request
 
     # Fetch all pipelines that the user can read.
     def execute
@@ -35,7 +37,7 @@ module Ci
 
           pipelines =
             if merge_request.persisted?
-              pipelines_using_cte
+              all_pipelines_for_merge_request
             else
               triggered_for_branch.for_sha(commit_shas)
             end
@@ -79,6 +81,17 @@ module Ci
       pipelines.joins(shas_table) # rubocop: disable CodeReuse/ActiveRecord
     end
 
+    def all_pipelines_for_merge_request
+      if Feature.enabled?(:decomposed_ci_query_in_pipelines_for_merge_request_finder, target_project, default_enabled: :yaml)
+        pipelines_for_merge_request = triggered_by_merge_request
+        pipelines_for_branch = triggered_for_branch.for_sha(recent_diff_head_shas(COMMITS_LIMIT))
+
+        Ci::Pipeline.from_union([pipelines_for_merge_request, pipelines_for_branch])
+      else
+        pipelines_using_cte
+      end
+    end
+
     # NOTE: this method returns only parent merge request pipelines.
     # Child merge request pipelines have a different source.
     def triggered_by_merge_request

app/models/merge_request.rb

@@ -1658,6 +1658,10 @@ class MergeRequest < ApplicationRecord
     service_class.new(project, current_user, id: id, report_type: report_type).execute(comparison_base_pipeline(identifier), actual_head_pipeline)
   end
 
+  def recent_diff_head_shas(limit = 100)
+    merge_request_diffs.recent(limit).pluck(:head_commit_sha)
+  end
+
   def all_commits
     MergeRequestDiffCommit
       .where(merge_request_diff: merge_request_diffs.recent)

app/models/merge_request_diff.rb

@@ -66,7 +66,7 @@ class MergeRequestDiff < ApplicationRecord
     joins(:merge_request).where(merge_requests: { target_project_id: project_id })
   end
 
-  scope :recent, -> { order(id: :desc).limit(100) }
+  scope :recent, -> (limit = 100) { order(id: :desc).limit(limit) }
 
   scope :files_in_database, -> do
     where(stored_externally: [false, nil]).where(arel_table[:files_count].gt(0))

config/feature_flags/development/decomposed_ci_query_in_pipelines_for_merge_request_finder.yml

+---
+name: decomposed_ci_query_in_pipelines_for_merge_request_finder
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68549
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/341341
+milestone: '14.4'
+type: development
+group: group::pipeline execution
+default_enabled: false

spec/finders/ci/pipelines_for_merge_request_finder_spec.rb

@@ -44,7 +44,7 @@ RSpec.describe Ci::PipelinesForMergeRequestFinder do
       let(:actor) { developer_in_both }
 
       it 'returns all pipelines' do
-        is_expected.to eq([pipeline_in_fork, pipeline_in_parent])
+        is_expected.to match_array([pipeline_in_fork, pipeline_in_parent])
       end
     end
 
@@ -52,7 +52,7 @@ RSpec.describe Ci::PipelinesForMergeRequestFinder do
       let(:actor) { reporter_in_parent_and_developer_in_fork }
 
       it 'returns all pipelines' do
-        is_expected.to eq([pipeline_in_fork, pipeline_in_parent])
+        is_expected.to match_array([pipeline_in_fork, pipeline_in_parent])
       end
     end
 
@@ -60,7 +60,7 @@ RSpec.describe Ci::PipelinesForMergeRequestFinder do
       let(:actor) { developer_in_parent }
 
       it 'returns pipelines in parent' do
-        is_expected.to eq([pipeline_in_parent])
+        is_expected.to match_array([pipeline_in_parent])
       end
     end
 
@@ -68,7 +68,7 @@ RSpec.describe Ci::PipelinesForMergeRequestFinder do
       let(:actor) { developer_in_fork }
 
       it 'returns pipelines in fork' do
-        is_expected.to eq([pipeline_in_fork])
+        is_expected.to match_array([pipeline_in_fork])
       end
     end
 
@@ -97,7 +97,7 @@ RSpec.describe Ci::PipelinesForMergeRequestFinder do
 
     shared_examples 'returning pipelines with proper ordering' do
       let!(:all_pipelines) do
-        merge_request.all_commit_shas.map do |sha|
+        merge_request.recent_diff_head_shas.map do |sha|
           create(:ci_empty_pipeline,
             project: project, sha: sha, ref: merge_request.source_branch)
         end
@@ -135,12 +135,92 @@ RSpec.describe Ci::PipelinesForMergeRequestFinder do
     end
 
     context 'when pipelines exist for the branch and merge request' do
+      shared_examples 'returns all pipelines for merge request' do
+        it 'returns merge request pipeline first' do
+          expect(subject.all).to eq([detached_merge_request_pipeline, branch_pipeline])
+        end
+
+        context 'when there are a branch pipeline and a merge request pipeline' do
+          let!(:branch_pipeline_2) do
+            create(:ci_pipeline, source: :push, project: project,
+                   ref: source_ref, sha: shas.first)
+          end
+
+          let!(:detached_merge_request_pipeline_2) do
+            create(:ci_pipeline, source: :merge_request_event, project: project,
+                   ref: source_ref, sha: shas.first, merge_request: merge_request)
+          end
+
+          it 'returns merge request pipelines first' do
+            expect(subject.all)
+              .to eq([detached_merge_request_pipeline_2,
+                      detached_merge_request_pipeline,
+                      branch_pipeline_2,
+                      branch_pipeline])
+          end
+        end
+
+        context 'when there are multiple merge request pipelines from the same branch' do
+          let!(:branch_pipeline_2) do
+            create(:ci_pipeline, source: :push, project: project,
+                   ref: source_ref, sha: shas.first)
+          end
+
+          let!(:branch_pipeline_with_sha_not_belonging_to_merge_request) do
+            create(:ci_pipeline, source: :push, project: project, ref: source_ref)
+          end
+
+          let!(:detached_merge_request_pipeline_2) do
+            create(:ci_pipeline, source: :merge_request_event, project: project,
+                   ref: source_ref, sha: shas.first, merge_request: merge_request_2)
+          end
+
+          let(:merge_request_2) do
+            create(:merge_request, source_project: project, source_branch: source_ref,
+                   target_project: project, target_branch: 'stable')
+          end
+
+          before do
+            shas.each.with_index do |sha, index|
+              create(:merge_request_diff_commit,
+                     merge_request_diff: merge_request_2.merge_request_diff,
+                     sha: sha, relative_order: index)
+            end
+          end
+
+          it 'returns only related merge request pipelines' do
+            expect(subject.all)
+              .to eq([detached_merge_request_pipeline,
+                      branch_pipeline_2,
+                      branch_pipeline])
+
+            expect(described_class.new(merge_request_2, nil).all)
+              .to match_array([detached_merge_request_pipeline_2, branch_pipeline_2, branch_pipeline])
+          end
+        end
+
+        context 'when detached merge request pipeline is run on head ref of the merge request' do
+          let!(:detached_merge_request_pipeline) do
+            create(:ci_pipeline, source: :merge_request_event, project: project,
+                   ref: merge_request.ref_path, sha: shas.second, merge_request: merge_request)
+          end
+
+          it 'sets the head ref of the merge request to the pipeline ref' do
+            expect(detached_merge_request_pipeline.ref).to match(%r{refs/merge-requests/\d+/head})
+          end
+
+          it 'includes the detached merge request pipeline even though the ref is custom path' do
+            expect(merge_request.all_pipelines).to include(detached_merge_request_pipeline)
+          end
+        end
+      end
+
       let(:source_ref) { 'feature' }
       let(:target_ref) { 'master' }
 
       let!(:branch_pipeline) do
         create(:ci_pipeline, source: :push, project: project,
-          ref: source_ref, sha: shas.second)
+               ref: source_ref, sha: merge_request.merge_request_diff.head_commit_sha)
       end
 
       let!(:tag_pipeline) do
@@ -149,97 +229,31 @@ RSpec.describe Ci::PipelinesForMergeRequestFinder do
 
       let!(:detached_merge_request_pipeline) do
         create(:ci_pipeline, source: :merge_request_event, project: project,
-          ref: source_ref, sha: shas.second, merge_request: merge_request)
+               ref: source_ref, sha: shas.second, merge_request: merge_request)
       end
 
       let(:merge_request) do
         create(:merge_request, source_project: project, source_branch: source_ref,
-          target_project: project, target_branch: target_ref)
+               target_project: project, target_branch: target_ref)
       end
 
       let(:project) { create(:project, :repository) }
       let(:shas) { project.repository.commits(source_ref, limit: 2).map(&:id) }
 
-      before do
-        create(:merge_request_diff_commit,
-          merge_request_diff: merge_request.merge_request_diff,
-          sha: shas.second, relative_order: 1)
-      end
-
-      it 'returns merge request pipeline first' do
-        expect(subject.all).to eq([detached_merge_request_pipeline, branch_pipeline])
-      end
-
-      context 'when there are a branch pipeline and a merge request pipeline' do
-        let!(:branch_pipeline_2) do
-          create(:ci_pipeline, source: :push, project: project,
-            ref: source_ref, sha: shas.first)
-        end
-
-        let!(:detached_merge_request_pipeline_2) do
-          create(:ci_pipeline, source: :merge_request_event, project: project,
-            ref: source_ref, sha: shas.first, merge_request: merge_request)
-        end
-
-        it 'returns merge request pipelines first' do
-          expect(subject.all)
-            .to eq([detached_merge_request_pipeline_2,
-                    detached_merge_request_pipeline,
-                    branch_pipeline_2,
-                    branch_pipeline])
-        end
-      end
-
-      context 'when there are multiple merge request pipelines from the same branch' do
-        let!(:branch_pipeline_2) do
-          create(:ci_pipeline, source: :push, project: project,
-            ref: source_ref, sha: shas.first)
-        end
-
-        let!(:detached_merge_request_pipeline_2) do
-          create(:ci_pipeline, source: :merge_request_event, project: project,
-            ref: source_ref, sha: shas.first, merge_request: merge_request_2)
-        end
-
-        let(:merge_request_2) do
-          create(:merge_request, source_project: project, source_branch: source_ref,
-            target_project: project, target_branch: 'stable')
-        end
-
+      context 'when `decomposed_ci_query_in_pipelines_for_merge_request_finder` feature flag enabled' do
         before do
-          shas.each.with_index do |sha, index|
-            create(:merge_request_diff_commit,
-              merge_request_diff: merge_request_2.merge_request_diff,
-              sha: sha, relative_order: index)
-          end
+          stub_feature_flags(decomposed_ci_query_in_pipelines_for_merge_request_finder: merge_request.target_project)
         end
 
-        it 'returns only related merge request pipelines' do
-          expect(subject.all)
-            .to eq([detached_merge_request_pipeline,
-                    branch_pipeline_2,
-                    branch_pipeline])
-
-          expect(described_class.new(merge_request_2, nil).all)
-            .to eq([detached_merge_request_pipeline_2,
-                    branch_pipeline_2,
-                    branch_pipeline])
-        end
+        it_behaves_like 'returns all pipelines for merge request'
       end
 
-      context 'when detached merge request pipeline is run on head ref of the merge request' do
-        let!(:detached_merge_request_pipeline) do
-          create(:ci_pipeline, source: :merge_request_event, project: project,
-            ref: merge_request.ref_path, sha: shas.second, merge_request: merge_request)
-        end
-
-        it 'sets the head ref of the merge request to the pipeline ref' do
-          expect(detached_merge_request_pipeline.ref).to match(%r{refs/merge-requests/\d+/head})
+      context 'when `decomposed_ci_query_in_pipelines_for_merge_request_finder` feature flag disabled' do
+        before do
+          stub_feature_flags(decomposed_ci_query_in_pipelines_for_merge_request_finder: false)
         end
 
-        it 'includes the detached merge request pipeline even though the ref is custom path' do
-          expect(merge_request.all_pipelines).to include(detached_merge_request_pipeline)
-        end
+        it_behaves_like 'returns all pipelines for merge request'
       end
     end
   end

spec/requests/api/graphql/project/merge_request/pipelines_spec.rb

@@ -39,7 +39,7 @@ RSpec.describe 'Query.project.mergeRequests.pipelines' do
 
     before do
       merge_requests.each do |mr|
-        shas = mr.all_commits.limit(2).pluck(:sha)
+        shas = mr.recent_diff_head_shas
 
         shas.each do |sha|
           create(:ci_pipeline, :success, project: project, ref: mr.source_branch, sha: sha)
@@ -52,7 +52,7 @@ RSpec.describe 'Query.project.mergeRequests.pipelines' do
 
       p_nodes = graphql_data_at(:project, :merge_requests, :nodes)
 
-      expect(p_nodes).to all(match('iid' => be_present, 'pipelines' => match('count' => 2)))
+      expect(p_nodes).to all(match('iid' => be_present, 'pipelines' => match('count' => 1)))
     end
 
     it 'is scalable', :request_store, :use_clean_rails_memory_store_caching do