Merge branch 'update-ci-build-merge-request' into 'master'

Update CI::Build#merge_request implementation

See merge request gitlab-org/gitlab!27968

app/models/ci/bridge.rb

@@ -3,7 +3,6 @@
 module Ci
   class Bridge < Ci::Processable
     include Ci::Contextable
-    include Ci::PipelineDelegator
     include Ci::Metadatable
     include Importable
     include AfterCommitQueue

app/models/ci/build.rb

@@ -4,7 +4,6 @@ module Ci
   class Build < Ci::Processable
     include Ci::Metadatable
     include Ci::Contextable
-    include Ci::PipelineDelegator
     include TokenAuthenticatable
     include AfterCommitQueue
     include ObjectStorage::BackgroundMove
@@ -591,13 +590,7 @@ module Ci
 
     def merge_request
       strong_memoize(:merge_request) do
-        merge_requests = MergeRequest.includes(:latest_merge_request_diff)
-          .where(source_branch: ref, source_project: pipeline.project)
-          .reorder(iid: :desc)
-
-        merge_requests.find do |merge_request|
-          merge_request.commit_shas.include?(pipeline.sha)
-        end
+        pipeline.all_merge_requests.order(iid: :asc).first
       end
     end
 

app/models/ci/processable.rb

@@ -51,6 +51,12 @@ module Ci
     validates :type, presence: true
     validates :scheduling_type, presence: true, on: :create, if: :validate_scheduling_type?
 
+    delegate :merge_request?,
+      :merge_request_ref?,
+      :legacy_detached_merge_request_pipeline?,
+      :merge_train_pipeline?,
+      to: :pipeline
+
     def aggregated_needs_names
       read_attribute(:aggregated_needs_names)
     end

app/models/concerns/ci/has_ref.rb

@@ -2,7 +2,7 @@
 
 ##
 # We will disable `ref` and `sha` attributes in `Ci::Build` in the future
-# and remove this module in favor of Ci::PipelineDelegator.
+# and remove this module in favor of Ci::Processable.
 module Ci
   module HasRef
     extend ActiveSupport::Concern

app/models/concerns/ci/pipeline_delegator.rb

-# frozen_string_literal: true
-
-##
-# This module is mainly used by child associations of `Ci::Pipeline` that needs to look up
-# single source of truth. For example, `Ci::Build` has `git_ref` method, which behaves
-# slightly different from `Ci::Pipeline`'s `git_ref`. This is very confusing as
-# the system could behave differently time to time.
-# We should have a single interface in `Ci::Pipeline` and access the method always.
-module Ci
-  module PipelineDelegator
-    extend ActiveSupport::Concern
-
-    included do
-      delegate :merge_request?,
-               :merge_request_ref?,
-               :legacy_detached_merge_request_pipeline?,
-               :merge_train_pipeline?, to: :pipeline
-    end
-  end
-end

changelogs/unreleased/update-ci-build-merge-request.yml

+---
+title: Use Ci::Pipeline#all_merge_requests.first as Ci::Build#merge_request
+merge_request: 27968
+author:
+type: fixed

ee/spec/models/ci/processable_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Ci::Processable do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:pipeline) { create(:ci_pipeline, project: project) }
+
+  let_it_be(:detached_merge_request_pipeline) do
+    create(:ci_pipeline, :detached_merge_request_pipeline, :with_job, project: project)
+  end
+
+  let_it_be(:legacy_detached_merge_request_pipeline) do
+    create(:ci_pipeline, :legacy_detached_merge_request_pipeline, :with_job, project: project)
+  end
+
+  let_it_be(:merged_result_pipeline) do
+    create(:ci_pipeline, :merged_result_pipeline, :with_job, project: project)
+  end
+
+  describe '#merge_train_pipeline?' do
+    subject { pipeline.processables.first.merge_train_pipeline? }
+
+    context 'in a detached merge request pipeline' do
+      let(:pipeline) { detached_merge_request_pipeline }
+
+      it { is_expected.to eq(pipeline.merge_train_pipeline?) }
+    end
+
+    context 'in a legacy detached merge_request_pipeline' do
+      let(:pipeline) { legacy_detached_merge_request_pipeline }
+
+      it { is_expected.to eq(pipeline.merge_train_pipeline?) }
+    end
+
+    context 'in a pipeline for merged results' do
+      let(:pipeline) { merged_result_pipeline }
+
+      it { is_expected.to eq(pipeline.merge_train_pipeline?) }
+    end
+  end
+end

spec/models/ci/bridge_spec.rb

@@ -17,8 +17,6 @@ describe Ci::Bridge do
     { trigger: { project: 'my/project', branch: 'master' } }
   end
 
-  it { is_expected.to include_module(Ci::PipelineDelegator) }
-
   it 'has many sourced pipelines' do
     expect(bridge).to have_many(:sourced_pipelines)
   end

spec/models/ci/build_spec.rb

@@ -37,8 +37,6 @@ describe Ci::Build do
   it { is_expected.to delegate_method(:merge_request_ref?).to(:pipeline) }
   it { is_expected.to delegate_method(:legacy_detached_merge_request_pipeline?).to(:pipeline) }
 
-  it { is_expected.to include_module(Ci::PipelineDelegator) }
-
   describe 'associations' do
     it 'has a bidirectional relationship with projects' do
       expect(described_class.reflect_on_association(:project).has_inverse?).to eq(:builds)
@@ -1818,64 +1816,65 @@ describe Ci::Build do
   end
 
   describe '#merge_request' do
-    def create_mr(build, pipeline, factory: :merge_request, created_at: Time.now)
-      create(factory, source_project: pipeline.project,
-                      target_project: pipeline.project,
-                      source_branch: build.ref,
-                      created_at: created_at)
-    end
+    subject { pipeline.builds.take.merge_request }
 
-    context 'when a MR has a reference to the pipeline' do
-      before do
-        @merge_request = create_mr(build, pipeline, factory: :merge_request)
+    context 'on a branch pipeline' do
+      let!(:pipeline) { create(:ci_pipeline, :with_job, project: project, ref: 'fix') }
 
-        commits = [double(id: pipeline.sha)]
-        allow(@merge_request).to receive(:commits).and_return(commits)
-        allow(MergeRequest).to receive_message_chain(:includes, :where, :reorder).and_return([@merge_request])
+      context 'with no merge request' do
+        it { is_expected.to be_nil }
       end
 
-      it 'returns the single associated MR' do
-        expect(build.merge_request.id).to eq(@merge_request.id)
-      end
-    end
+      context 'with an open merge request from the same ref name' do
+        let!(:merge_request) { create(:merge_request, source_project: project, source_branch: 'fix') }
 
-    context 'when there is not a MR referencing the pipeline' do
-      it 'returns nil' do
-        expect(build.merge_request).to be_nil
-      end
-    end
+        # If no diff exists, the pipeline commit was not part of the merge
+        # request and may have simply incidentally used the same ref name.
+        context 'without a merge request diff containing the pipeline commit' do
+          it { is_expected.to be_nil }
+        end
 
-    context 'when more than one MR have a reference to the pipeline' do
-      before do
-        @merge_request = create_mr(build, pipeline, factory: :merge_request)
-        @merge_request.close!
-        @merge_request2 = create_mr(build, pipeline, factory: :merge_request)
+        # If the merge request was truly opened from the branch that the
+        # pipeline ran on, that head sha will be present in a diff.
+        context 'with a merge request diff containing the pipeline commit' do
+          let!(:mr_diff) { create(:merge_request_diff, merge_request: merge_request) }
+          let!(:mr_diff_commit) { create(:merge_request_diff_commit, sha: build.sha, merge_request_diff: mr_diff) }
 
-        commits = [double(id: pipeline.sha)]
-        allow(@merge_request).to receive(:commits).and_return(commits)
-        allow(@merge_request2).to receive(:commits).and_return(commits)
-        allow(MergeRequest).to receive_message_chain(:includes, :where, :reorder).and_return([@merge_request, @merge_request2])
+          it { is_expected.to eq(merge_request) }
+        end
       end
 
-      it 'returns the first MR' do
-        expect(build.merge_request.id).to eq(@merge_request.id)
+      context 'with multiple open merge requests' do
+        let!(:merge_request)  { create(:merge_request, source_project: project, source_branch: 'fix') }
+        let!(:mr_diff)        { create(:merge_request_diff, merge_request: merge_request) }
+        let!(:mr_diff_commit) { create(:merge_request_diff_commit, sha: build.sha, merge_request_diff: mr_diff) }
+
+        let!(:new_merge_request)  { create(:merge_request, source_project: project, source_branch: 'fix', target_branch: 'staging') }
+        let!(:new_mr_diff)        { create(:merge_request_diff, merge_request: new_merge_request) }
+        let!(:new_mr_diff_commit) { create(:merge_request_diff_commit, sha: build.sha, merge_request_diff: new_mr_diff) }
+
+        it 'returns the first merge request' do
+          expect(subject).to eq(merge_request)
+        end
       end
     end
 
-    context 'when a Build is created after the MR' do
-      before do
-        @merge_request = create_mr(build, pipeline, factory: :merge_request_with_diffs)
-        pipeline2 = create(:ci_pipeline, project: project)
-        @build2 = create(:ci_build, pipeline: pipeline2)
+    context 'on a detached merged request pipeline' do
+      let(:pipeline) { create(:ci_pipeline, :detached_merge_request_pipeline, :with_job) }
 
-        allow(@merge_request).to receive(:commit_shas)
-          .and_return([pipeline.sha, pipeline2.sha])
-        allow(MergeRequest).to receive_message_chain(:includes, :where, :reorder).and_return([@merge_request])
-      end
+      it { is_expected.to eq(pipeline.merge_request) }
+    end
 
-      it 'returns the current MR' do
-        expect(@build2.merge_request.id).to eq(@merge_request.id)
-      end
+    context 'on a legacy detached merged request pipeline' do
+      let(:pipeline) { create(:ci_pipeline, :legacy_detached_merge_request_pipeline, :with_job) }
+
+      it { is_expected.to eq(pipeline.merge_request) }
+    end
+
+    context 'on a pipeline for merged results' do
+      let(:pipeline) { create(:ci_pipeline, :merged_result_pipeline, :with_job) }
+
+      it { is_expected.to eq(pipeline.merge_request) }
     end
   end
 

spec/models/ci/processable_spec.rb

@@ -6,6 +6,18 @@ describe Ci::Processable do
   let_it_be(:project) { create(:project) }
   let_it_be(:pipeline) { create(:ci_pipeline, project: project) }
 
+  let_it_be(:detached_merge_request_pipeline) do
+    create(:ci_pipeline, :detached_merge_request_pipeline, :with_job, project: project)
+  end
+
+  let_it_be(:legacy_detached_merge_request_pipeline) do
+    create(:ci_pipeline, :legacy_detached_merge_request_pipeline, :with_job, project: project)
+  end
+
+  let_it_be(:merged_result_pipeline) do
+    create(:ci_pipeline, :merged_result_pipeline, :with_job, project: project)
+  end
+
   describe '#aggregated_needs_names' do
     let(:with_aggregated_needs) { pipeline.processables.select_with_aggregated_needs(project) }
 
@@ -155,4 +167,70 @@ describe Ci::Processable do
       end
     end
   end
+
+  describe '#merge_request?' do
+    subject { pipeline.processables.first.merge_request? }
+
+    context 'in a detached merge request pipeline' do
+      let(:pipeline) { detached_merge_request_pipeline }
+
+      it { is_expected.to eq(pipeline.merge_request?) }
+    end
+
+    context 'in a legacy detached merge_request_pipeline' do
+      let(:pipeline) { legacy_detached_merge_request_pipeline }
+
+      it { is_expected.to eq(pipeline.merge_request?) }
+    end
+
+    context 'in a pipeline for merged results' do
+      let(:pipeline) { merged_result_pipeline }
+
+      it { is_expected.to eq(pipeline.merge_request?) }
+    end
+  end
+
+  describe '#merge_request_ref?' do
+    subject { pipeline.processables.first.merge_request_ref? }
+
+    context 'in a detached merge request pipeline' do
+      let(:pipeline) { detached_merge_request_pipeline }
+
+      it { is_expected.to eq(pipeline.merge_request_ref?) }
+    end
+
+    context 'in a legacy detached merge_request_pipeline' do
+      let(:pipeline) { legacy_detached_merge_request_pipeline }
+
+      it { is_expected.to eq(pipeline.merge_request_ref?) }
+    end
+
+    context 'in a pipeline for merged results' do
+      let(:pipeline) { merged_result_pipeline }
+
+      it { is_expected.to eq(pipeline.merge_request_ref?) }
+    end
+  end
+
+  describe '#legacy_detached_merge_request_pipeline?' do
+    subject { pipeline.processables.first.legacy_detached_merge_request_pipeline? }
+
+    context 'in a detached merge request pipeline' do
+      let(:pipeline) { detached_merge_request_pipeline }
+
+      it { is_expected.to eq(pipeline.legacy_detached_merge_request_pipeline?) }
+    end
+
+    context 'in a legacy detached merge_request_pipeline' do
+      let(:pipeline) { legacy_detached_merge_request_pipeline }
+
+      it { is_expected.to eq(pipeline.legacy_detached_merge_request_pipeline?) }
+    end
+
+    context 'in a pipeline for merged results' do
+      let(:pipeline) { merged_result_pipeline }
+
+      it { is_expected.to eq(pipeline.legacy_detached_merge_request_pipeline?) }
+    end
+  end
 end