Merge branch '254801-refactor-naming' into 'master'

Refactor naming of pipeline object hierarchy methods

See merge request gitlab-org/gitlab!63978

app/models/ci/pipeline.rb

@@ -904,7 +904,7 @@ module Ci
 
     def same_family_pipeline_ids
       ::Gitlab::Ci::PipelineObjectHierarchy.new(
-        self.class.default_scoped.where(id: root_ancestor), options: { same_project: true }
+        self.class.default_scoped.where(id: root_ancestor), options: { project_condition: :same }
       ).base_and_descendants.select(:id)
     end
 
@@ -925,29 +925,34 @@ module Ci
       Environment.where(id: environment_ids)
     end
 
-    # Without using `unscoped`, caller scope is also included into the query.
-    # Using `unscoped` here will be redundant after Rails 6.1
+    # With multi-project and parent-child pipelines
+    def self_and_upstreams
+      object_hierarchy.base_and_ancestors
+    end
+
+    # With multi-project and parent-child pipelines
+    def self_with_upstreams_and_downstreams
+      object_hierarchy.all_objects
+    end
+
+    # With only parent-child pipelines
+    def self_and_ancestors
+      object_hierarchy(project_condition: :same).base_and_ancestors
+    end
+
+    # With only parent-child pipelines
     def self_and_descendants
-      ::Gitlab::Ci::PipelineObjectHierarchy
-        .new(self.class.unscoped.where(id: id), options: { same_project: true })
-        .base_and_descendants
+      object_hierarchy(project_condition: :same).base_and_descendants
     end
 
     def root_ancestor
       return self unless child?
 
-      Gitlab::Ci::PipelineObjectHierarchy
-        .new(self.class.unscoped.where(id: id), options: { same_project: true })
+      object_hierarchy(project_condition: :same)
         .base_and_ancestors(hierarchy_order: :desc)
         .first
     end
 
-    def self_with_ancestors_and_descendants(same_project: false)
-      ::Gitlab::Ci::PipelineObjectHierarchy
-        .new(self.class.unscoped.where(id: id), options: { same_project: same_project })
-        .all_objects
-    end
-
     def bridge_triggered?
       source_bridge.present?
     end
@@ -1207,14 +1212,6 @@ module Ci
       self.ci_ref = Ci::Ref.ensure_for(self)
     end
 
-    def base_and_ancestors(same_project: false)
-      # Without using `unscoped`, caller scope is also included into the query.
-      # Using `unscoped` here will be redundant after Rails 6.1
-      ::Gitlab::Ci::PipelineObjectHierarchy
-        .new(self.class.unscoped.where(id: id), options: { same_project: same_project })
-        .base_and_ancestors
-    end
-
     # We need `base_and_ancestors` in a specific order to "break" when needed.
     # If we use `find_each`, then the order is broken.
     # rubocop:disable Rails/FindEach
@@ -1225,7 +1222,7 @@ module Ci
         source_bridge.pending!
         Ci::AfterRequeueJobService.new(project, current_user).execute(source_bridge) # rubocop:disable CodeReuse/ServiceClass
       else
-        base_and_ancestors.includes(:source_bridge).each do |pipeline|
+        self_and_upstreams.includes(:source_bridge).each do |pipeline|
           break unless pipeline.bridge_waiting?
 
           pipeline.source_bridge.pending!
@@ -1308,6 +1305,13 @@ module Ci
 
       project.repository.keep_around(self.sha, self.before_sha)
     end
+
+    # Without using `unscoped`, caller scope is also included into the query.
+    # Using `unscoped` here will be redundant after Rails 6.1
+    def object_hierarchy(options = {})
+      ::Gitlab::Ci::PipelineObjectHierarchy
+        .new(self.class.unscoped.where(id: id), options: options)
+    end
   end
 end
 

app/services/ci/create_downstream_pipeline_service.rb

@@ -120,7 +120,7 @@ module Ci
       return false if @bridge.triggers_child_pipeline?
 
       if Feature.enabled?(:ci_drop_cyclical_triggered_pipelines, @bridge.project, default_enabled: :yaml)
-        pipeline_checksums = @bridge.pipeline.base_and_ancestors.filter_map do |pipeline|
+        pipeline_checksums = @bridge.pipeline.self_and_upstreams.filter_map do |pipeline|
           config_checksum(pipeline) unless pipeline.child?
         end
 
@@ -131,7 +131,7 @@ module Ci
     def has_max_descendants_depth?
       return false unless @bridge.triggers_child_pipeline?
 
-      ancestors_of_new_child = @bridge.pipeline.base_and_ancestors(same_project: true)
+      ancestors_of_new_child = @bridge.pipeline.self_and_ancestors
       ancestors_of_new_child.count > MAX_DESCENDANTS_DEPTH
     end
 

app/services/ci/expire_pipeline_cache_service.rb

@@ -77,7 +77,7 @@ module Ci
         store.touch(path)
       end
 
-      pipeline.self_with_ancestors_and_descendants.each do |relative_pipeline|
+      pipeline.self_with_upstreams_and_downstreams.each do |relative_pipeline|
         store.touch(project_pipeline_path(relative_pipeline.project, relative_pipeline))
         store.touch(graphql_pipeline_path(relative_pipeline))
         store.touch(graphql_pipeline_sha_path(relative_pipeline.sha))

lib/gitlab/ci/pipeline_object_hierarchy.rb

@@ -21,7 +21,7 @@ module Gitlab
         middle_table[:source_pipeline_id].eq(objects_table[:id]).and(
           middle_table[:pipeline_id].eq(cte.table[:id])
         ).and(
-          same_project_condition
+          project_condition
         )
       end
 
@@ -29,15 +29,15 @@ module Gitlab
         middle_table[:pipeline_id].eq(objects_table[:id]).and(
           middle_table[:source_pipeline_id].eq(cte.table[:id])
         ).and(
-          same_project_condition
+          project_condition
         )
       end
 
-      def same_project_condition
-        if options[:same_project]
-          middle_table[:source_project_id].eq(middle_table[:project_id])
-        else
-          Arel.sql('TRUE')
+      def project_condition
+        case options[:project_condition]
+        when :same then middle_table[:source_project_id].eq(middle_table[:project_id])
+        when :different then middle_table[:source_project_id].not_eq(middle_table[:project_id])
+        else Arel.sql('TRUE')
         end
       end
     end

spec/lib/gitlab/ci/pipeline_object_hierarchy_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do
   let_it_be(:cousin_parent) { create(:ci_pipeline, project: project) }
   let_it_be(:cousin) { create(:ci_pipeline, project: project) }
   let_it_be(:triggered_pipeline) { create(:ci_pipeline) }
+  let_it_be(:triggered_child_pipeline) { create(:ci_pipeline) }
 
   before_all do
     create_source_pipeline(ancestor, parent)
@@ -19,19 +20,20 @@ RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do
     create_source_pipeline(parent, child)
     create_source_pipeline(cousin_parent, cousin)
     create_source_pipeline(child, triggered_pipeline)
+    create_source_pipeline(triggered_pipeline, triggered_child_pipeline)
   end
 
   describe '#base_and_ancestors' do
     it 'includes the base and its ancestors' do
       relation = described_class.new(::Ci::Pipeline.where(id: parent.id),
-                                     options: { same_project: true }).base_and_ancestors
+                                     options: { project_condition: :same }).base_and_ancestors
 
       expect(relation).to contain_exactly(ancestor, parent)
     end
 
     it 'can find ancestors upto a certain level' do
       relation = described_class.new(::Ci::Pipeline.where(id: child.id),
-                                     options: { same_project: true }).base_and_ancestors(upto: ancestor.id)
+                                     options: { project_condition: :same }).base_and_ancestors(upto: ancestor.id)
 
       expect(relation).to contain_exactly(parent, child)
     end
@@ -39,7 +41,7 @@ RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do
     describe 'hierarchy_order option' do
       let(:relation) do
         described_class.new(::Ci::Pipeline.where(id: child.id),
-                            options: { same_project: true }).base_and_ancestors(hierarchy_order: hierarchy_order)
+                            options: { project_condition: :same }).base_and_ancestors(hierarchy_order: hierarchy_order)
       end
 
       context ':asc' do
@@ -63,15 +65,32 @@ RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do
   describe '#base_and_descendants' do
     it 'includes the base and its descendants' do
       relation = described_class.new(::Ci::Pipeline.where(id: parent.id),
-                                     options: { same_project: true }).base_and_descendants
+                                     options: { project_condition: :same }).base_and_descendants
 
       expect(relation).to contain_exactly(parent, child)
     end
 
+    context 'when project_condition: :different' do
+      it "includes the base and other project pipelines" do
+        relation = described_class.new(::Ci::Pipeline.where(id: child.id),
+                                       options: { project_condition: :different }).base_and_descendants
+
+        expect(relation).to contain_exactly(child, triggered_pipeline, triggered_child_pipeline)
+      end
+    end
+
+    context 'when project_condition: nil' do
+      it "includes the base and its descendants with other project pipeline" do
+        relation = described_class.new(::Ci::Pipeline.where(id: parent.id)).base_and_descendants
+
+        expect(relation).to contain_exactly(parent, child, triggered_pipeline, triggered_child_pipeline)
+      end
+    end
+
     context 'when with_depth is true' do
       let(:relation) do
         described_class.new(::Ci::Pipeline.where(id: ancestor.id),
-                            options: { same_project: true }).base_and_descendants(with_depth: true)
+                            options: { project_condition: :same }).base_and_descendants(with_depth: true)
       end
 
       it 'includes depth in the results' do
@@ -91,21 +110,51 @@ RSpec.describe Gitlab::Ci::PipelineObjectHierarchy do
   end
 
   describe '#all_objects' do
-    it 'includes its ancestors and descendants' do
-      relation = described_class.new(::Ci::Pipeline.where(id: parent.id),
-                                     options: { same_project: true }).all_objects
+    context 'when passing ancestors_base' do
+      let(:options) { { project_condition: project_condition } }
+      let(:ancestors_base) { ::Ci::Pipeline.where(id: child.id) }
+
+      subject(:relation) { described_class.new(ancestors_base, options: options).all_objects }
 
-      expect(relation).to contain_exactly(ancestor, parent, child)
+      context 'when project_condition: :same' do
+        let(:project_condition) { :same }
+
+        it "includes its ancestors and descendants" do
+          expect(relation).to contain_exactly(ancestor, parent, child)
+        end
+      end
+
+      context 'when project_condition: :different' do
+        let(:project_condition) { :different }
+
+        it "includes the base and other project pipelines" do
+          expect(relation).to contain_exactly(child, triggered_pipeline, triggered_child_pipeline)
+        end
+      end
     end
 
-    it 'returns all family tree' do
-      relation = described_class.new(
-        ::Ci::Pipeline.where(id: child.id),
-        described_class.new(::Ci::Pipeline.where(id: child.id), options: { same_project: true }).base_and_ancestors,
-        options: { same_project: true }
-      ).all_objects
+    context 'when passing ancestors_base and descendants_base' do
+      let(:options) { { project_condition: project_condition } }
+      let(:ancestors_base) { ::Ci::Pipeline.where(id: child.id) }
+      let(:descendants_base) { described_class.new(::Ci::Pipeline.where(id: child.id), options: options).base_and_ancestors }
+
+      subject(:relation) { described_class.new(ancestors_base, descendants_base, options: options).all_objects }
+
+      context 'when project_condition: :same' do
+        let(:project_condition) { :same }
 
-      expect(relation).to contain_exactly(ancestor, parent, cousin_parent, child, cousin)
+        it 'returns all family tree' do
+          expect(relation).to contain_exactly(ancestor, parent, cousin_parent, child, cousin)
+        end
+      end
+
+      context 'when project_condition: :different' do
+        let(:project_condition) { :different }
+
+        it "includes the base and other project pipelines" do
+          expect(relation).to contain_exactly(child, triggered_pipeline, triggered_child_pipeline)
+        end
+      end
     end
   end
 end

spec/models/ci/pipeline_spec.rb

@@ -4355,16 +4355,14 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
     end
   end
 
-  describe '#base_and_ancestors' do
-    subject { pipeline.base_and_ancestors(same_project: same_project) }
+  describe '#self_and_upstreams' do
+    subject(:self_and_upstreams) { pipeline.self_and_upstreams }
 
     let_it_be(:pipeline) { create(:ci_pipeline, :created) }
 
-    let(:same_project) { false }
-
     context 'when pipeline is not child nor parent' do
       it 'returns just the pipeline itself' do
-        expect(subject).to contain_exactly(pipeline)
+        expect(self_and_upstreams).to contain_exactly(pipeline)
       end
     end
 
@@ -4378,7 +4376,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
       end
 
       it 'returns parent and self' do
-        expect(subject).to contain_exactly(parent, pipeline)
+        expect(self_and_upstreams).to contain_exactly(parent, pipeline)
       end
     end
 
@@ -4390,7 +4388,7 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
       end
 
       it 'returns self' do
-        expect(subject).to contain_exactly(pipeline)
+        expect(self_and_upstreams).to contain_exactly(pipeline)
       end
     end
 
@@ -4406,11 +4404,11 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
       end
 
       it 'returns self, parent and ancestor' do
-        expect(subject).to contain_exactly(ancestor, parent, pipeline)
+        expect(self_and_upstreams).to contain_exactly(ancestor, parent, pipeline)
       end
     end
 
-    context 'when pipeline is a triggered pipeline' do
+    context 'when pipeline is a triggered pipeline from a different project' do
       let_it_be(:pipeline) { create(:ci_pipeline, :created) }
 
       let(:upstream) { create(:ci_pipeline, project: create(:project)) }
@@ -4419,18 +4417,41 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
         create_source_pipeline(upstream, pipeline)
       end
 
-      context 'same_project: false' do
-        it 'returns upstream and self' do
-          expect(subject).to contain_exactly(pipeline, upstream)
-        end
+      it 'returns upstream and self' do
+        expect(self_and_upstreams).to contain_exactly(pipeline, upstream)
       end
+    end
+  end
 
-      context 'same_project: true' do
-        let(:same_project) { true }
+  describe '#self_and_ancestors' do
+    subject(:self_and_ancestors) { pipeline.self_and_ancestors }
 
-        it 'returns self' do
-          expect(subject).to contain_exactly(pipeline)
-        end
+    context 'when pipeline is child' do
+      let(:pipeline) { create(:ci_pipeline, :created) }
+      let(:parent) { create(:ci_pipeline) }
+      let(:sibling) { create(:ci_pipeline) }
+
+      before do
+        create_source_pipeline(parent, pipeline)
+        create_source_pipeline(parent, sibling)
+      end
+
+      it 'returns parent and self' do
+        expect(self_and_ancestors).to contain_exactly(parent, pipeline)
+      end
+    end
+
+    context 'when pipeline is a triggered pipeline from a different project' do
+      let_it_be(:pipeline) { create(:ci_pipeline, :created) }
+
+      let(:upstream) { create(:ci_pipeline, project: create(:project)) }
+
+      before do
+        create_source_pipeline(upstream, pipeline)
+      end
+
+      it 'returns only self' do
+        expect(self_and_ancestors).to contain_exactly(pipeline)
       end
     end
   end
@@ -4468,15 +4489,18 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
         end
 
         context 'when the parent pipeline has a dependent upstream pipeline' do
-          let!(:upstream_bridge) do
-            create_bridge(create(:ci_pipeline, project: create(:project)), parent_pipeline, true)
-          end
+          let(:upstream_pipeline) { create(:ci_pipeline, project: create(:project)) }
+          let!(:upstream_bridge) { create_bridge(upstream_pipeline, parent_pipeline, true) }
+
+          let(:upstream_upstream_pipeline) { create(:ci_pipeline, project: create(:project)) }
+          let!(:upstream_upstream_bridge) { create_bridge(upstream_upstream_pipeline, upstream_pipeline, true) }
 
           it 'marks all source bridges as pending' do
             reset_bridge
 
             expect(bridge.reload).to be_pending
             expect(upstream_bridge.reload).to be_pending
+            expect(upstream_upstream_bridge.reload).to be_pending
           end
         end
       end