Various improvements to hierarchy sorting

- Rename ordered_group_clusters_for_project ->
ancestor_clusters_for_clusterable
- Improve name of order option. It makes much more sense to have `hierarchy_order: :asc`
and `hierarchy_order: :desc`
- Allow ancestor_clusters_for_clusterable for group
- Re-use code already present in Project

8 files changed, 70 insertions, 38 deletions

diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb
index 73cae8d3b25..e7d0471813a 100644
--- a/app/models/clusters/cluster.rb
+++ b/app/models/clusters/cluster.rb
@@ -93,12 +93,8 @@ module Clusters
       where('NOT EXISTS (?)', subquery)
     end
 
-    # Returns an ordered list of group clusters order from clusters of closest
-    # group up to furthest ancestor group
-    def self.ordered_group_clusters_for_project(project_id)
-      project_groups = ::Group.joins(:projects).where(projects: { id: project_id })
-      hierarchy_groups = Gitlab::GroupHierarchy.new(project_groups)
-        .base_and_ancestors(depth: :desc)
+    def self.ancestor_clusters_for_clusterable(clusterable, hierarchy_order: :asc)
+      hierarchy_groups = clusterable.ancestors_upto(hierarchy_order: hierarchy_order)
 
       hierarchy_groups.flat_map(&:clusters)
     end
diff --git a/app/models/concerns/deployment_platform.rb b/app/models/concerns/deployment_platform.rb
index d785e0d505f..ad981f4a8a3 100644
--- a/app/models/concerns/deployment_platform.rb
+++ b/app/models/concerns/deployment_platform.rb
@@ -32,8 +32,8 @@ module DeploymentPlatform
 
   # EE would override this and utilize environment argument
   def find_group_cluster_platform_kubernetes(environment: nil)
-    Clusters::Cluster.enabled.default_environment.ordered_group_clusters_for_project(id)
-      .last&.platform_kubernetes
+    Clusters::Cluster.enabled.default_environment.ancestor_clusters_for_clusterable(self)
+      .first&.platform_kubernetes
   end
 
   def find_kubernetes_service_integration
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 11b03846f0b..c38310ed62b 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -192,9 +192,9 @@ class Namespace < ActiveRecord::Base
 
   # returns all ancestors upto but excluding the given namespace
   # when no namespace is given, all ancestors upto the top are returned
-  def ancestors_upto(top = nil)
+  def ancestors_upto(top = nil, hierarchy_order: nil)
     Gitlab::GroupHierarchy.new(self.class.where(id: id))
-      .ancestors(upto: top)
+      .ancestors(upto: top, hierarchy_order: hierarchy_order)
   end
 
   def self_and_ancestors
diff --git a/app/models/project.rb b/app/models/project.rb
index a3580961d42..602347d4bac 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -552,9 +552,9 @@ class Project < ActiveRecord::Base
 
   # returns all ancestor-groups upto but excluding the given namespace
   # when no namespace is given, all ancestors upto the top are returned
-  def ancestors_upto(top = nil)
+  def ancestors_upto(top = nil, hierarchy_order: nil)
     Gitlab::GroupHierarchy.new(Group.where(id: namespace_id))
-      .base_and_ancestors(upto: top)
+      .base_and_ancestors(upto: top, hierarchy_order: hierarchy_order)
   end
 
   def lfs_enabled?
diff --git a/lib/gitlab/group_hierarchy.rb b/lib/gitlab/group_hierarchy.rb
index 2502887065f..97cbdc6cb39 100644
--- a/lib/gitlab/group_hierarchy.rb
+++ b/lib/gitlab/group_hierarchy.rb
@@ -34,8 +34,8 @@ module Gitlab
     # reached. So all ancestors *lower* than the specified ancestor will be
     # included.
     # rubocop: disable CodeReuse/ActiveRecord
-    def ancestors(upto: nil)
-      base_and_ancestors(upto: upto).where.not(id: ancestors_base.select(:id))
+    def ancestors(upto: nil, hierarchy_order: nil)
+      base_and_ancestors(upto: upto, hierarchy_order: hierarchy_order).where.not(id: ancestors_base.select(:id))
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
@@ -46,16 +46,17 @@ module Gitlab
     # reached. So all ancestors *lower* than the specified acestor will be
     # included.
     #
-    # Passing an `depth` with either `:asc` or `:desc` will cause the recursive
-    # query to use a depth column to order by depth (`:asc` returns most nested
-    # group to root; `desc` returns opposite order). We define 1 as the depth
-    # for the base and increment as we go up each parent.
+    # Passing a `hierarchy_order` with either `:asc` or `:desc` will cause the
+    # recursive query order from most nested group to root or from the root
+    # ancestor to most nested group respectively. This uses a `depth` column
+    # where `1` is defined as the depth for the base and increment as we go up
+    # each parent.
     # rubocop: disable CodeReuse/ActiveRecord
-    def base_and_ancestors(upto: nil, depth: nil)
+    def base_and_ancestors(upto: nil, hierarchy_order: nil)
       return ancestors_base unless Group.supports_nested_groups?
 
-      recursive_query = base_and_ancestors_cte(upto, depth).apply_to(model.all)
-      recursive_query = recursive_query.order(depth: depth) if depth
+      recursive_query = base_and_ancestors_cte(upto, hierarchy_order).apply_to(model.all)
+      recursive_query = recursive_query.order(depth: hierarchy_order) if hierarchy_order
 
       read_only(recursive_query)
     end
@@ -117,11 +118,12 @@ module Gitlab
     private
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def base_and_ancestors_cte(stop_id = nil, depth = nil)
+    def base_and_ancestors_cte(stop_id = nil, hierarchy_order = nil)
       cte = SQL::RecursiveCTE.new(:base_and_ancestors)
+      depth_column = :depth
 
       base_query = ancestors_base.except(:order)
-      base_query = base_query.select('1 AS depth', groups_table[Arel.star]) if depth
+      base_query = base_query.select("1 as #{depth_column}", groups_table[Arel.star]) if hierarchy_order
 
       cte << base_query
 
@@ -131,7 +133,7 @@ module Gitlab
         .where(groups_table[:id].eq(cte.table[:parent_id]))
         .except(:order)
 
-      parent_query = parent_query.select(cte.table[:depth] + 1, groups_table[Arel.star]) if depth
+      parent_query = parent_query.select(cte.table[depth_column] + 1, groups_table[Arel.star]) if hierarchy_order
       parent_query = parent_query.where(cte.table[:parent_id].not_eq(stop_id)) if stop_id
 
       cte << parent_query
diff --git a/spec/lib/gitlab/group_hierarchy_spec.rb b/spec/lib/gitlab/group_hierarchy_spec.rb
index edc03ff2e59..f3de7adcec7 100644
--- a/spec/lib/gitlab/group_hierarchy_spec.rb
+++ b/spec/lib/gitlab/group_hierarchy_spec.rb
@@ -35,13 +35,25 @@ describe Gitlab::GroupHierarchy, :postgresql do
         .to raise_error(ActiveRecord::ReadOnlyRecord)
     end
 
-    context 'with depth option' do
+    describe 'hierarchy_order option' do
       let(:relation) do
-        described_class.new(Group.where(id: child2.id)).base_and_ancestors(depth: :asc)
+        described_class.new(Group.where(id: child2.id)).base_and_ancestors(hierarchy_order: hierarchy_order)
       end
 
-      it 'orders by depth' do
-        expect(relation.map(&:depth)).to eq([1, 2, 3])
+      context ':asc' do
+        let(:hierarchy_order) { :asc }
+
+        it 'orders by child to parent' do
+          expect(relation).to eq([child2, child1, parent])
+        end
+      end
+
+      context ':desc' do
+        let(:hierarchy_order) { :desc }
+
+        it 'orders by parent to child' do
+          expect(relation).to eq([parent, child1, child2])
+        end
       end
     end
   end
diff --git a/spec/models/clusters/cluster_spec.rb b/spec/models/clusters/cluster_spec.rb
index 229956b56b2..adb0ea3a444 100644
--- a/spec/models/clusters/cluster_spec.rb
+++ b/spec/models/clusters/cluster_spec.rb
@@ -253,17 +253,21 @@ describe Clusters::Cluster do
     end
   end
 
-  describe '.ordered_group_clusters_for_project' do
+  describe '.ancestor_clusters_for_clusterable' do
     let(:group_cluster) { create(:cluster, :provided_by_gcp, :group) }
     let(:group) { group_cluster.group }
+    let(:hierarchy_order) { :desc }
+    let(:clusterable) { project }
 
-    subject { described_class.ordered_group_clusters_for_project(project.id) }
+    subject do
+      described_class.ancestor_clusters_for_clusterable(clusterable, hierarchy_order: hierarchy_order)
+    end
 
     context 'when project does not belong to this group' do
       let(:project) { create(:project, group: create(:group)) }
 
       it 'returns nothing' do
-        expect(subject).to be_empty
+        is_expected.to be_empty
       end
     end
 
@@ -271,11 +275,11 @@ describe Clusters::Cluster do
       let(:project) { create(:project, group: group) }
 
       it 'returns the group cluster' do
-        expect(subject).to eq([group_cluster])
+        is_expected.to eq([group_cluster])
       end
     end
 
-    context 'when sub-group has configured kubernetes cluster', :postgresql do
+    context 'when sub-group has configured kubernetes cluster', :nested_groups do
       let(:sub_group_cluster) { create(:cluster, :provided_by_gcp, :group) }
       let(:sub_group) { sub_group_cluster.group }
       let(:project) { create(:project, group: sub_group) }
@@ -284,18 +288,26 @@ describe Clusters::Cluster do
         sub_group.update!(parent: group)
       end
 
-      it 'returns clusters in order, ascending the hierachy' do
-        expect(subject).to eq([group_cluster, sub_group_cluster])
+      it 'returns clusters in order, descending the hierachy' do
+        is_expected.to eq([group_cluster, sub_group_cluster])
+      end
+
+      context 'for a group' do
+        let(:clusterable) { sub_group }
+
+        it 'returns clusters in order for a group' do
+          is_expected.to eq([group_cluster])
+        end
       end
     end
 
-    context 'cluster_scope arg' do
+    context 'scope chaining' do
       let(:project) { create(:project, group: group) }
 
-      subject { described_class.none.ordered_group_clusters_for_project(project.id) }
+      subject { described_class.none.ancestor_clusters_for_clusterable(project) }
 
       it 'returns nothing' do
-        expect(subject).to be_empty
+        is_expected.to be_empty
       end
     end
   end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 8b1743def72..68dcb8b3e22 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2117,6 +2117,16 @@ describe Project do
     it 'includes ancestors upto but excluding the given ancestor' do
       expect(project.ancestors_upto(parent)).to contain_exactly(child2, child)
     end
+
+    describe 'with hierarchy_order' do
+      it 'returns ancestors ordered by descending hierarchy' do
+        expect(project.ancestors_upto(hierarchy_order: :desc)).to eq([parent, child, child2])
+      end
+
+      it 'can be used with upto option' do
+        expect(project.ancestors_upto(parent, hierarchy_order: :desc)).to eq([child, child2])
+      end
+    end
   end
 
   describe '#lfs_enabled?' do