Commit adb8f676 authored by Thong Kuah's avatar Thong Kuah
Browse files

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
parent afce416eb849
......@@ -93,12 +93,8 @@ class Cluster < ActiveRecord::Base
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
......
......@@ -32,8 +32,8 @@ def find_group_cluster_platform_kubernetes_with_feature_guard(environment: nil)
# 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
......
......@@ -192,9 +192,9 @@ def ancestors
# 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
......
......@@ -552,9 +552,9 @@ def group_ids
# 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?
......
......@@ -34,8 +34,8 @@ def descendants
# 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 @@ def ancestors(upto: nil)
# 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 @@ def all_groups
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 @@ def base_and_ancestors_cte(stop_id = nil, depth = nil)
.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
......
......@@ -35,13 +35,25 @@
.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
......
......@@ -253,17 +253,21 @@
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 @@
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 @@
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
......
......@@ -2117,6 +2117,16 @@ def create_build(new_pipeline = pipeline, name = 'test')
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
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment