Refactor merge request pipeline relationsrefactor-merge-request-pipeline-relations

Improve

Fix more

Fix

Remove unnecessary

Simplify more

Improve

ok

ok

7 files changed, 55 insertions, 47 deletions

diff --git a/app/mailers/emails/pipelines.rb b/app/mailers/emails/pipelines.rb
index 31e183640ad..fb57c0da34d 100644
--- a/app/mailers/emails/pipelines.rb
+++ b/app/mailers/emails/pipelines.rb
@@ -15,7 +15,7 @@ module Emails
     def pipeline_mail(pipeline, recipients, status)
       @project = pipeline.project
       @pipeline = pipeline
-      @merge_request = pipeline.merge_requests.first
+      @merge_request = pipeline.merge_requests_as_head_pipeline.first
       add_headers
 
       # We use bcc here because we don't want to generate this emails for a
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index c902e49ee6d..daf226b7311 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -46,7 +46,7 @@ module Ci
     delegate :terminal_specification, to: :runner_session, allow_nil: true
     delegate :gitlab_deploy_token, to: :project
     delegate :trigger_short_token, to: :trigger_request, allow_nil: true
-    delegate :merge_request?, to: :pipeline
+    delegate :merge_request?, :first_merge_request, to: :pipeline
 
     ##
     # Since Gitlab 11.5, deployments records started being created right after
@@ -463,22 +463,6 @@ module Ci
       { trace_sections: true }
     end
 
-    def merge_request
-      return @merge_request if defined?(@merge_request)
-
-      @merge_request ||=
-        begin
-          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
-        end
-    end
-
     def repo_url
       return unless token
 
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index d4586219333..28046b72e94 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -21,7 +21,7 @@ module Ci
     belongs_to :user
     belongs_to :auto_canceled_by, class_name: 'Ci::Pipeline'
     belongs_to :pipeline_schedule, class_name: 'Ci::PipelineSchedule'
-    belongs_to :merge_request, class_name: 'MergeRequest'
+    belongs_to :merge_request_as_merge_request_pipeline, foreign_key: "merge_request_id", class_name: 'MergeRequest'
 
     has_internal_id :iid, scope: :project, presence: false, init: ->(s) do
       s&.project&.all_pipelines&.maximum(:iid) || s&.project&.all_pipelines&.count
@@ -39,7 +39,7 @@ module Ci
 
     # Merge requests for which the current pipeline is running against
     # the merge request's latest commit.
-    has_many :merge_requests, foreign_key: "head_pipeline_id"
+    has_many :merge_requests_as_head_pipeline, foreign_key: "head_pipeline_id", class_name: 'MergeRequest'
 
     has_many :pending_builds, -> { pending }, foreign_key: :commit_id, class_name: 'Ci::Build'
     has_many :retryable_builds, -> { latest.failed_or_canceled.includes(:project) }, foreign_key: :commit_id, class_name: 'Ci::Build'
@@ -185,14 +185,15 @@ module Ci
     end
 
     scope :for_user, -> (user) { where(user: user) }
+    scope :for_sha, -> (sha) { where(sha: sha) }
 
-    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
+    scope :for_merge_request, -> (merge_request) do
+      union = Gitlab::SQL::Union.new(
+        [Ci::Pipeline.triggered_by_merge_request(merge_request).select(:id),
+         Ci::Pipeline.indeterministic_pipelines_for_merge_request(merge_request).select(:id)],
+         remove_duplicates: false)
+
+      Ci::Pipeline.where("id IN (#{union.to_sql})").sort_by_merge_request_pipelines
     end
 
     scope :triggered_by_merge_request, -> (merge_request) do
@@ -211,6 +212,11 @@ module Ci
       triggered_by_merge_request(merge_request).where(target_sha: merge_request.target_branch_sha)
     end
 
+    scope :indeterministic_pipelines_for_merge_request, -> (merge_request) do
+      ci_sources.where.not(source: :merge_request)
+        .where(ref: merge_request.source_branch, project: merge_request.source_project)
+    end
+
     # Returns the pipelines in descending order (= newest first), optionally
     # limited to a number of references.
     #
@@ -298,8 +304,10 @@ 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.latest_for_merge_request(merge_request)
+      for_merge_request(merge_request)
+        .for_sha([merge_request.diff_head_sha, merge_request.merge_ref_sha])
+        .first
     end
 
     def self.ci_sources_values
@@ -643,10 +651,10 @@ module Ci
         variables.append(key: 'CI_COMMIT_TITLE', value: git_commit_full_title.to_s)
         variables.append(key: 'CI_COMMIT_DESCRIPTION', value: git_commit_description.to_s)
 
-        if merge_request? && merge_request
+        if triggered_by_merge_request?
           variables.append(key: 'CI_MERGE_REQUEST_SOURCE_BRANCH_SHA', value: source_sha.to_s)
           variables.append(key: 'CI_MERGE_REQUEST_TARGET_BRANCH_SHA', value: target_sha.to_s)
-          variables.concat(merge_request.predefined_variables)
+          variables.concat(merge_request_as_merge_request_pipeline.predefined_variables)
         end
       end
     end
@@ -672,14 +680,21 @@ module Ci
 
     # All the merge requests for which the current pipeline runs/ran against
     def all_merge_requests
+      return @all_merge_requests if defined?(@all_merge_requests)
+      return if tag?
+
       @all_merge_requests ||=
-        if merge_request?
+        if triggered_by_merge_request?
           MergeRequest.where(id: merge_request_id)
         else
-          MergeRequest.where(source_project_id: project_id, source_branch: ref)
+          MergeRequest.with_indeterministic_pipeline(self)
         end
     end
 
+    def first_merge_request
+      @first_merge_request ||= all_merge_requests.try(:first)
+    end
+
     def detailed_status(current_user)
       Gitlab::Ci::Status::Pipeline::Factory
         .new(self, current_user)
@@ -719,7 +734,7 @@ module Ci
     def modified_paths
       strong_memoize(:modified_paths) do
         if merge_request?
-          merge_request.modified_paths
+          merge_request_as_merge_request_pipeline.modified_paths
         elsif branch_updated?
           push_details.modified_paths
         end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 1468ae1c34a..0db42519215 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -185,6 +185,12 @@ class MergeRequest < ActiveRecord::Base
   scope :unassigned, -> { where("assignee_id IS NULL") }
   scope :assigned_to, ->(u) { where(assignee_id: u.id)}
 
+  scope :with_indeterministic_pipeline, -> (pipeline) do
+    raise ArgumentError, 'This is deterministic pipeline' if pipeline.merge_request?
+
+    where(source_branch: pipeline.ref, source_project: pipeline.project)
+  end
+
   participant :assignee
 
   after_save :keep_around_commit
@@ -1130,9 +1136,7 @@ class MergeRequest < ActiveRecord::Base
   def all_pipelines(shas: all_commit_shas)
     return Ci::Pipeline.none unless source_project
 
-    @all_pipelines ||=
-      source_project.ci_pipelines
-                    .for_merge_request(self, source_branch, all_commit_shas)
+    @all_pipelines ||= Ci::Pipeline.for_merge_request(self)
   end
 
   def update_head_pipeline
@@ -1383,10 +1387,15 @@ class MergeRequest < ActiveRecord::Base
     source_project.repository.squash_in_progress?(id)
   end
 
+  def merge_ref_sha
+    strong_memoize(:merge_ref_sha) do
+      project.repository.commit(merge_ref_path)
+    end
+  end
+
   private
 
   def find_actual_head_pipeline
-    source_project&.ci_pipelines
-                  &.latest_for_merge_request(self, source_branch, diff_head_sha)
+    Ci::Pipeline.latest_for_merge_request(self)
   end
 end
diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb
index 9ddce0d2c80..0112e9f6535 100644
--- a/app/serializers/build_details_entity.rb
+++ b/app/serializers/build_details_entity.rb
@@ -49,14 +49,14 @@ class BuildDetailsEntity < JobEntity
     terminal_project_job_path(project, build)
   end
 
-  expose :merge_request, if: -> (*) { can?(current_user, :read_merge_request, build.merge_request) } do
+  expose :merge_request, if: -> (*) { can?(current_user, :read_merge_request, build.first_merge_request) } do
     expose :iid do |build|
-      build.merge_request.iid
+      build.first_merge_request.iid
     end
 
     expose :path do |build|
-      project_merge_request_path(build.merge_request.project,
-                                 build.merge_request)
+      project_merge_request_path(build.first_merge_request.project,
+                                 build.first_merge_request)
     end
   end
 
diff --git a/app/workers/pipeline_metrics_worker.rb b/app/workers/pipeline_metrics_worker.rb
index c2fbfd2b3a5..0ddad43b8d5 100644
--- a/app/workers/pipeline_metrics_worker.rb
+++ b/app/workers/pipeline_metrics_worker.rb
@@ -30,6 +30,6 @@ class PipelineMetricsWorker
   # rubocop: enable CodeReuse/ActiveRecord
 
   def merge_requests(pipeline)
-    pipeline.merge_requests.map(&:id)
+    pipeline.merge_requests_as_head_pipeline.map(&:id)
   end
 end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 81ff727b458..50c90f00b43 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -1648,13 +1648,13 @@ describe Ci::Build do
       end
 
       it 'returns the single associated MR' do
-        expect(build.merge_request.id).to eq(@merge_request.id)
+        expect(build.first_merge_request.id).to eq(@merge_request.id)
       end
     end
 
     context 'when there is not a MR referencing the pipeline' do
       it 'returns nil' do
-        expect(build.merge_request).to be_nil
+        expect(build.first_merge_request).to be_nil
       end
     end
 
@@ -1671,7 +1671,7 @@ describe Ci::Build do
       end
 
       it 'returns the first MR' do
-        expect(build.merge_request.id).to eq(@merge_request.id)
+        expect(build.first_merge_request.id).to eq(@merge_request.id)
       end
     end