From b913169d887bab6b02469cac4d245e2612deb76d Mon Sep 17 00:00:00 2001
From: Shinya Maeda <shinya@gitlab.com>
Date: Fri, 1 Mar 2019 15:18:08 +0900
Subject: [PATCH] Make all_pipelines method compatible with pipelines for merge
 requests

Make it sane

Include merge ref head

Fix union

Improve a bit

Add spec

remove

add spec

Add changelog

fix coding offence

Apply suggestion to spec/models/merge_request_spec.rb

ok

ok

Fix

Fix spec

Fix spec

fix

Simplify the things

Memoize

OK

a
---
 app/models/ci/pipeline.rb                     |  29 ++-
 app/models/merge_request.rb                   |  17 +-
 .../unreleased/use-only-all-pipelines.yml     |   5 +
 spec/factories/merge_requests.rb              |   4 +-
 spec/models/ci/pipeline_spec.rb               | 170 ++++++++++++++++--
 spec/models/merge_request_spec.rb             |  66 ++++++-
 spec/serializers/pipeline_entity_spec.rb      |   2 +
 spec/serializers/pipeline_serializer_spec.rb  |   6 +-
 8 files changed, 256 insertions(+), 43 deletions(-)
 create mode 100644 changelogs/unreleased/use-only-all-pipelines.yml

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index ca9725f7a04..adffdc0355e 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -13,6 +13,7 @@ module Ci
     include EnumWithNil
     include HasRef
     include ShaAttribute
+    include FromUnion
 
     sha_attribute :source_sha
     sha_attribute :target_sha
@@ -185,32 +186,30 @@ module Ci
     end
 
     scope :for_user, -> (user) { where(user: user) }
-
-    scope :for_merge_request, -> (merge_request, ref, sha) do
-      ##
-      # We have to filter out unrelated MR pipelines.
-      # When merge request is empty, it selects general pipelines, such as push sourced pipelines.
-      # When merge request is matched, it selects MR pipelines.
-      where(merge_request: [nil, merge_request], ref: ref, sha: sha)
-        .sort_by_merge_request_pipelines
-    end
+    scope :for_sha, -> (sha) { where(sha: sha) }
+    scope :for_source_sha, -> (source_sha) { where(source_sha: source_sha) }
+    scope :for_sha_or_source_sha, -> (sha) { for_sha(sha).or(for_source_sha(sha)) }
 
     scope :triggered_by_merge_request, -> (merge_request) do
       where(source: :merge_request_event, merge_request: merge_request)
     end
 
-    scope :detached_merge_request_pipelines, -> (merge_request) do
-      triggered_by_merge_request(merge_request).where(target_sha: nil)
+    scope :detached_merge_request_pipelines, -> (merge_request, sha) do
+      triggered_by_merge_request(merge_request).for_sha(sha)
     end
 
-    scope :merge_request_pipelines, -> (merge_request) do
-      triggered_by_merge_request(merge_request).where.not(target_sha: nil)
+    scope :merge_request_pipelines, -> (merge_request, source_sha) do
+      triggered_by_merge_request(merge_request).for_source_sha(source_sha)
     end
 
     scope :mergeable_merge_request_pipelines, -> (merge_request) do
       triggered_by_merge_request(merge_request).where(target_sha: merge_request.target_branch_sha)
     end
 
+    scope :triggered_for_branch, -> (ref) do
+      where(source: branch_pipeline_sources).where(ref: ref, tag: false)
+    end
+
     # Returns the pipelines in descending order (= newest first), optionally
     # limited to a number of references.
     #
@@ -298,8 +297,8 @@ module Ci
       sources.reject { |source| source == "external" }.values
     end
 
-    def self.latest_for_merge_request(merge_request, ref, sha)
-      for_merge_request(merge_request, ref, sha).first
+    def self.branch_pipeline_sources
+      @branch_pipeline_sources ||= sources.reject { |source| source == 'merge_request_event' }.values
     end
 
     def self.ci_sources_values
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index af8cb37bfb6..aeb6acf0ac0 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1149,12 +1149,18 @@ class MergeRequest < ActiveRecord::Base
     diverged_commits_count > 0
   end
 
-  def all_pipelines(shas: all_commit_shas)
+  def all_pipelines
     return Ci::Pipeline.none unless source_project
 
-    @all_pipelines ||=
-      source_project.ci_pipelines
-                    .for_merge_request(self, source_branch, all_commit_shas)
+    shas = all_commit_shas
+
+    strong_memoize(:all_pipelines) do
+      Ci::Pipeline.from_union(
+        [source_project.ci_pipelines.merge_request_pipelines(self, shas),
+         source_project.ci_pipelines.detached_merge_request_pipelines(self, shas),
+         source_project.ci_pipelines.triggered_for_branch(source_branch).for_sha(shas)],
+         remove_duplicates: false).sort_by_merge_request_pipelines
+    end
   end
 
   def update_head_pipeline
@@ -1389,8 +1395,7 @@ class MergeRequest < ActiveRecord::Base
   private
 
   def find_actual_head_pipeline
-    source_project&.ci_pipelines
-                  &.latest_for_merge_request(self, source_branch, diff_head_sha)
+    all_pipelines.for_sha_or_source_sha(diff_head_sha).first
   end
 
   def source_project_variables
diff --git a/changelogs/unreleased/use-only-all-pipelines.yml b/changelogs/unreleased/use-only-all-pipelines.yml
new file mode 100644
index 00000000000..68364d2a923
--- /dev/null
+++ b/changelogs/unreleased/use-only-all-pipelines.yml
@@ -0,0 +1,5 @@
+---
+title: Refactor all_pipelines in Merge request
+merge_request: 25676
+author:
+type: other
diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb
index 18f724770b5..ecd7ea65fb7 100644
--- a/spec/factories/merge_requests.rb
+++ b/spec/factories/merge_requests.rb
@@ -106,7 +106,9 @@ FactoryBot.define do
         merge_request.merge_request_pipelines << build(:ci_pipeline,
           source: :merge_request_event,
           merge_request: merge_request,
-          project: merge_request.source_project)
+          project: merge_request.source_project,
+          ref: merge_request.source_branch,
+          sha: merge_request.source_branch_sha)
       end
     end
 
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index d0b42d103a5..7eeaa7a18ef 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -130,22 +130,118 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
+  describe '.for_sha' do
+    subject { described_class.for_sha(sha) }
+
+    let(:sha) { 'abc' }
+    let!(:pipeline) { create(:ci_pipeline, sha: 'abc') }
+
+    it 'returns the pipeline' do
+      is_expected.to contain_exactly(pipeline)
+    end
+
+    context 'when argument is array' do
+      let(:sha) { %w[abc def] }
+      let!(:pipeline_2) { create(:ci_pipeline, sha: 'def') }
+
+      it 'returns the pipelines' do
+        is_expected.to contain_exactly(pipeline, pipeline_2)
+      end
+    end
+
+    context 'when sha is empty' do
+      let(:sha) { nil }
+
+      it 'does not return anything' do
+        is_expected.to be_empty
+      end
+    end
+  end
+
+  describe '.for_source_sha' do
+    subject { described_class.for_source_sha(source_sha) }
+
+    let(:source_sha) { 'abc' }
+    let!(:pipeline) { create(:ci_pipeline, source_sha: 'abc') }
+
+    it 'returns the pipeline' do
+      is_expected.to contain_exactly(pipeline)
+    end
+
+    context 'when argument is array' do
+      let(:source_sha) { %w[abc def] }
+      let!(:pipeline_2) { create(:ci_pipeline, source_sha: 'def') }
+
+      it 'returns the pipelines' do
+        is_expected.to contain_exactly(pipeline, pipeline_2)
+      end
+    end
+
+    context 'when source_sha is empty' do
+      let(:source_sha) { nil }
+
+      it 'does not return anything' do
+        is_expected.to be_empty
+      end
+    end
+  end
+
+  describe '.for_sha_or_source_sha' do
+    subject { described_class.for_sha_or_source_sha(sha) }
+
+    let(:sha) { 'abc' }
+
+    context 'when sha is matched' do
+      let!(:pipeline) { create(:ci_pipeline, sha: sha) }
+
+      it 'returns the pipeline' do
+        is_expected.to contain_exactly(pipeline)
+      end
+    end
+
+    context 'when source sha is matched' do
+      let!(:pipeline) { create(:ci_pipeline, source_sha: sha) }
+
+      it 'returns the pipeline' do
+        is_expected.to contain_exactly(pipeline)
+      end
+    end
+
+    context 'when both sha and source sha are not matched' do
+      let!(:pipeline) { create(:ci_pipeline, sha: 'bcd', source_sha: 'bcd') }
+
+      it 'does not return anything' do
+        is_expected.to be_empty
+      end
+    end
+  end
+
   describe '.detached_merge_request_pipelines' do
-    subject { described_class.detached_merge_request_pipelines(merge_request) }
+    subject { described_class.detached_merge_request_pipelines(merge_request, sha) }
 
     let!(:pipeline) do
-      create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, target_sha: target_sha)
+      create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, sha: merge_request.diff_head_sha)
     end
 
     let(:merge_request) { create(:merge_request) }
-    let(:target_sha) { nil }
+    let(:sha) { merge_request.diff_head_sha }
 
     it 'returns detached merge request pipelines' do
       is_expected.to eq([pipeline])
     end
 
-    context 'when target sha exists' do
-      let(:target_sha) { merge_request.target_branch_sha }
+    context 'when sha does not exist' do
+      let(:sha) { 'abc' }
+
+      it 'returns empty array' do
+        is_expected.to be_empty
+      end
+    end
+
+    context 'when pipeline is merge request pipeline' do
+      let!(:pipeline) do
+        create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, source_sha: merge_request.diff_head_sha)
+      end
 
       it 'returns empty array' do
         is_expected.to be_empty
@@ -173,21 +269,31 @@ describe Ci::Pipeline, :mailer do
   end
 
   describe '.merge_request_pipelines' do
-    subject { described_class.merge_request_pipelines(merge_request) }
+    subject { described_class.merge_request_pipelines(merge_request, source_sha) }
 
     let!(:pipeline) do
-      create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, target_sha: target_sha)
+      create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, source_sha: merge_request.diff_head_sha)
     end
 
     let(:merge_request) { create(:merge_request) }
-    let(:target_sha) { merge_request.target_branch_sha }
+    let(:source_sha) { merge_request.diff_head_sha }
 
     it 'returns merge pipelines' do
       is_expected.to eq([pipeline])
     end
 
-    context 'when target sha is empty' do
-      let(:target_sha) { nil }
+    context 'when source sha is empty' do
+      let(:source_sha) { nil }
+
+      it 'returns empty array' do
+        is_expected.to be_empty
+      end
+    end
+
+    context 'when pipeline is detached merge request pipeline' do
+      let!(:pipeline) do
+        create(:ci_pipeline, source: :merge_request_event, merge_request: merge_request, sha: merge_request.diff_head_sha)
+      end
 
       it 'returns empty array' do
         is_expected.to be_empty
@@ -256,6 +362,50 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
+  describe '.triggered_for_branch' do
+    subject { described_class.triggered_for_branch(ref) }
+
+    let(:project) { create(:project, :repository) }
+    let(:ref) { 'feature' }
+    let!(:pipeline) { create(:ci_pipeline, ref: ref) }
+
+    it 'returns the pipeline' do
+      is_expected.to eq([pipeline])
+    end
+
+    context 'when sha is not specified' do
+      it 'returns the pipeline' do
+        expect(described_class.triggered_for_branch(ref)).to eq([pipeline])
+      end
+    end
+
+    context 'when pipeline is triggered for tag' do
+      let(:ref) { 'v1.1.0' }
+      let!(:pipeline) { create(:ci_pipeline, ref: ref, tag: true) }
+
+      it 'does not return the pipeline' do
+        is_expected.to be_empty
+      end
+    end
+
+    context 'when pipeline is triggered for merge_request' do
+      let!(:merge_request) do
+        create(:merge_request,
+          :with_merge_request_pipeline,
+          source_project: project,
+          source_branch: ref,
+          target_project: project,
+          target_branch: 'master')
+      end
+
+      let(:pipeline) { merge_request.merge_request_pipelines.first }
+
+      it 'does not return the pipeline' do
+        is_expected.to be_empty
+      end
+    end
+  end
+
   describe '.merge_request_event' do
     subject { described_class.merge_request_event }
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index a35d3f14df8..42c49e330cc 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1350,7 +1350,7 @@ describe MergeRequest do
                sha: shas.second)
       end
 
-      let!(:merge_request_pipeline) do
+      let!(:detached_merge_request_pipeline) do
         create(:ci_pipeline,
                source: :merge_request_event,
                project: project,
@@ -1376,7 +1376,7 @@ describe MergeRequest do
 
       it 'returns merge request pipeline first' do
         expect(merge_request.all_pipelines)
-          .to eq([merge_request_pipeline,
+          .to eq([detached_merge_request_pipeline,
                   branch_pipeline])
       end
 
@@ -1389,7 +1389,7 @@ describe MergeRequest do
                  sha: shas.first)
         end
 
-        let!(:merge_request_pipeline_2) do
+        let!(:detached_merge_request_pipeline_2) do
           create(:ci_pipeline,
                  source: :merge_request_event,
                  project: project,
@@ -1400,8 +1400,8 @@ describe MergeRequest do
 
         it 'returns merge request pipelines first' do
           expect(merge_request.all_pipelines)
-            .to eq([merge_request_pipeline_2,
-                    merge_request_pipeline,
+            .to eq([detached_merge_request_pipeline_2,
+                    detached_merge_request_pipeline,
                     branch_pipeline_2,
                     branch_pipeline])
         end
@@ -1416,7 +1416,7 @@ describe MergeRequest do
                  sha: shas.first)
         end
 
-        let!(:merge_request_pipeline_2) do
+        let!(:detached_merge_request_pipeline_2) do
           create(:ci_pipeline,
                  source: :merge_request_event,
                  project: project,
@@ -1439,16 +1439,35 @@ describe MergeRequest do
 
         it 'returns only related merge request pipelines' do
           expect(merge_request.all_pipelines)
-            .to eq([merge_request_pipeline,
+            .to eq([detached_merge_request_pipeline,
                     branch_pipeline_2,
                     branch_pipeline])
 
           expect(merge_request_2.all_pipelines)
-            .to eq([merge_request_pipeline_2,
+            .to eq([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
   end
 
@@ -1489,6 +1508,37 @@ describe MergeRequest do
       end
     end
 
+    context 'when detached merge request pipeline is run on head ref of the merge request' do
+      let!(:pipeline) do
+        create(:ci_pipeline,
+               source: :merge_request_event,
+               project: merge_request.source_project,
+               ref: merge_request.ref_path,
+               sha: sha,
+               merge_request: merge_request)
+      end
+
+      let(:sha) { merge_request.diff_head_sha }
+
+      it 'sets the head ref of the merge request to the pipeline ref' do
+        expect(pipeline.ref).to match(%r{refs/merge-requests/\d+/head})
+      end
+
+      it 'updates correctly even though the target branch name of the merge request is different from the pipeline ref' do
+        expect { subject }
+          .to change { merge_request.reload.head_pipeline }
+          .from(nil).to(pipeline)
+      end
+
+      context 'when sha is not HEAD of the source branch' do
+        let(:sha) { merge_request.diff_base_sha }
+
+        it 'does not update head pipeline' do
+          expect { subject }.not_to change { merge_request.reload.head_pipeline }
+        end
+      end
+    end
+
     context 'when there are no pipelines with the diff head sha' do
       it 'does not update the head pipeline' do
         expect { subject }
diff --git a/spec/serializers/pipeline_entity_spec.rb b/spec/serializers/pipeline_entity_spec.rb
index 11040862129..d510293ae38 100644
--- a/spec/serializers/pipeline_entity_spec.rb
+++ b/spec/serializers/pipeline_entity_spec.rb
@@ -4,12 +4,14 @@ describe PipelineEntity do
   include Gitlab::Routing
 
   set(:user) { create(:user) }
+  set(:project) { create(:project) }
   let(:request) { double('request') }
 
   before do
     stub_not_protect_default_branch
 
     allow(request).to receive(:current_user).and_return(user)
+    allow(request).to receive(:project).and_return(project)
   end
 
   let(:entity) do
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index a21487938a0..cead75c0895 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/spec/serializers/pipeline_serializer_spec.rb
@@ -5,7 +5,7 @@ describe PipelineSerializer do
   set(:user) { create(:user) }
 
   let(:serializer) do
-    described_class.new(current_user: user)
+    described_class.new(current_user: user, project: project)
   end
 
   before do
@@ -106,7 +106,7 @@ describe PipelineSerializer do
           target_project: project,
           target_branch: 'master',
           source_project: project,
-          source_branch: 'feature-1')
+          source_branch: 'feature')
       end
 
       let!(:merge_request_2) do
@@ -115,7 +115,7 @@ describe PipelineSerializer do
           target_project: project,
           target_branch: 'master',
           source_project: project,
-          source_branch: 'feature-2')
+          source_branch: '2-mb-file')
       end
 
       before do