Prevent false positives in Ci::Pipeline#all_merge_requests

Add a scope for checking MergeRequstDiffCommit shas to ensure that the
pipeline ran for some historical state of the merge request. We have
encountered situations where only matching on project and ref produce a
false positive: https://gitlab.com/gitlab-org/gitlab/issues/201947

app/models/ci/pipeline.rb

@@ -736,6 +736,7 @@ module Ci
           MergeRequest.where(id: merge_request_id)
         else
           MergeRequest.where(source_project_id: project_id, source_branch: ref)
+            .by_commit_sha(sha)
         end
     end
 

changelogs/unreleased/filter-pipeline-merge-requests-by-sha.yml

+---
+title: Prevent false positives in Ci::Pipeline#all_merge_requests
+merge_request: 28800
+author:
+type: fixed

ee/spec/serializers/pipeline_serializer_spec.rb

@@ -7,7 +7,7 @@ describe PipelineSerializer do
   let_it_be(:user) { create(:user) }
 
   let(:serializer) do
-    described_class.new(current_user: user)
+    described_class.new(current_user: user, project: project)
   end
 
   subject { serializer.represent(pipeline, details: true) }

spec/factories/ci/pipelines.rb

@@ -5,7 +5,7 @@ FactoryBot.define do
   factory :ci_empty_pipeline, class: 'Ci::Pipeline' do
     source { :push }
     ref { 'master' }
-    sha { '97de212e80737a608d939f648d959671fb0a0142' }
+    sha { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' }
     status { 'pending' }
     add_attribute(:protected) { false }
 

spec/features/dashboard/projects_spec.rb

@@ -244,13 +244,15 @@ describe 'Dashboard Projects' do
 
     ActiveRecord::QueryRecorder.new { visit dashboard_projects_path }.count
 
-    # There are three known N+1 queries:
+    # There are seven known N+1 queries: https://gitlab.com/gitlab-org/gitlab/-/issues/214037
     # 1. Project#open_issues_count
     # 2. Project#open_merge_requests_count
     # 3. Project#forks_count
-    #
-    # In addition, ProjectsHelper#load_pipeline_status also adds an
-    # additional query.
-    expect { visit dashboard_projects_path }.not_to exceed_query_limit(control_count + 4)
+    # 4. ProjectsHelper#load_pipeline_status
+    # 5. RendersMemberAccess#preload_max_member_access_for_collection
+    # 6. User#max_member_access_for_project_ids
+    # 7. CommitWithPipeline#last_pipeline
+
+    expect { visit dashboard_projects_path }.not_to exceed_query_limit(control_count + 7)
   end
 end

spec/features/projects/pipelines/pipeline_spec.rb

@@ -133,15 +133,8 @@ describe 'Pipeline', :js do
 
       context 'when there are two related merge requests' do
         before do
-          create(:merge_request,
-            source_project: project,
-            source_branch: pipeline.ref,
-            target_branch: 'feature-1')
-
-          create(:merge_request,
-            source_project: project,
-            source_branch: pipeline.ref,
-            target_branch: 'feature-2')
+          create(:merge_request, source_project: project, source_branch: pipeline.ref)
+          create(:merge_request, source_project: project, source_branch: pipeline.ref, target_branch: 'fix')
         end
 
         it 'links to the most recent related merge request' do

spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb

@@ -14,7 +14,7 @@ describe Resolvers::MergeRequestPipelinesResolver do
       sha: merge_request.diff_head_sha
     )
   end
-  let_it_be(:other_project_pipeline) { create(:ci_pipeline, project: merge_request.source_project) }
+  let_it_be(:other_project_pipeline) { create(:ci_pipeline, project: merge_request.source_project, ref: 'other-ref') }
   let_it_be(:other_pipeline) { create(:ci_pipeline) }
   let(:current_user) { create(:user) }
 

spec/models/ci/pipeline_spec.rb

@@ -2367,18 +2367,31 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
-  describe "#all_merge_requests" do
+  describe '#all_merge_requests' do
     let(:project) { create(:project) }
 
     shared_examples 'a method that returns all merge requests for a given pipeline' do
       let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: pipeline_project, ref: 'master') }
 
-      it "returns all merge requests having the same source branch" do
+      it 'returns all merge requests having the same source branch and the pipeline sha' do
         merge_request = create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: pipeline.ref)
 
+        create(:merge_request_diff, merge_request: merge_request).tap do |diff|
+          create(:merge_request_diff_commit, merge_request_diff: diff, sha: pipeline.sha)
+        end
+
         expect(pipeline.all_merge_requests).to eq([merge_request])
       end
 
+      it "doesn't return merge requests having the same source branch without the pipeline sha" do
+        merge_request = create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: pipeline.ref)
+        create(:merge_request_diff, merge_request: merge_request).tap do |diff|
+          create(:merge_request_diff_commit, merge_request_diff: diff, sha: 'unrelated')
+        end
+
+        expect(pipeline.all_merge_requests).to be_empty
+      end
+
       it "doesn't return merge requests having a different source branch" do
         create(:merge_request, source_project: pipeline_project, target_project: project, source_branch: 'feature', target_branch: 'master')
 

spec/presenters/ci/pipeline_presenter_spec.rb

@@ -236,7 +236,7 @@ describe Ci::PipelinePresenter do
 
     context 'for a branch pipeline with two open MRs' do
       let!(:one) { create(:merge_request, source_project: project, source_branch: pipeline.ref) }
-      let!(:two) { create(:merge_request, source_project: project, source_branch: pipeline.ref, target_branch: 'wip') }
+      let!(:two) { create(:merge_request, source_project: project, source_branch: pipeline.ref, target_branch: 'fix') }
 
       it { is_expected.to contain_exactly(one, two) }
     end

spec/serializers/build_details_entity_spec.rb

@@ -15,7 +15,7 @@ describe BuildDetailsEntity do
     let(:project) { create(:project, :repository) }
     let(:pipeline) { create(:ci_pipeline, project: project) }
     let(:build) { create(:ci_build, :failed, pipeline: pipeline) }
-    let(:request) { double('request') }
+    let(:request) { double('request', project: project) }
 
     let(:entity) do
       described_class.new(build, request: request,