Commit f85440e6 authored by Thong Kuah's avatar Thong Kuah

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 d54791e0
...@@ -93,12 +93,8 @@ module Clusters ...@@ -93,12 +93,8 @@ module Clusters
where('NOT EXISTS (?)', subquery) where('NOT EXISTS (?)', subquery)
end end
# Returns an ordered list of group clusters order from clusters of closest def self.ancestor_clusters_for_clusterable(clusterable, hierarchy_order: :asc)
# group up to furthest ancestor group hierarchy_groups = clusterable.ancestors_upto(hierarchy_order: hierarchy_order)
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)
hierarchy_groups.flat_map(&:clusters) hierarchy_groups.flat_map(&:clusters)
end end
......
...@@ -32,8 +32,8 @@ module DeploymentPlatform ...@@ -32,8 +32,8 @@ module DeploymentPlatform
# EE would override this and utilize environment argument # EE would override this and utilize environment argument
def find_group_cluster_platform_kubernetes(environment: nil) def find_group_cluster_platform_kubernetes(environment: nil)
Clusters::Cluster.enabled.default_environment.ordered_group_clusters_for_project(id) Clusters::Cluster.enabled.default_environment.ancestor_clusters_for_clusterable(self)
.last&.platform_kubernetes .first&.platform_kubernetes
end end
def find_kubernetes_service_integration def find_kubernetes_service_integration
......
...@@ -192,9 +192,9 @@ class Namespace < ActiveRecord::Base ...@@ -192,9 +192,9 @@ class Namespace < ActiveRecord::Base
# returns all ancestors upto but excluding the given namespace # returns all ancestors upto but excluding the given namespace
# when no namespace is given, all ancestors upto the top are returned # 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)) Gitlab::GroupHierarchy.new(self.class.where(id: id))
.ancestors(upto: top) .ancestors(upto: top, hierarchy_order: hierarchy_order)
end end
def self_and_ancestors def self_and_ancestors
......
...@@ -552,9 +552,9 @@ class Project < ActiveRecord::Base ...@@ -552,9 +552,9 @@ class Project < ActiveRecord::Base
# returns all ancestor-groups upto but excluding the given namespace # returns all ancestor-groups upto but excluding the given namespace
# when no namespace is given, all ancestors upto the top are returned # 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)) Gitlab::GroupHierarchy.new(Group.where(id: namespace_id))
.base_and_ancestors(upto: top) .base_and_ancestors(upto: top, hierarchy_order: hierarchy_order)
end end
def lfs_enabled? def lfs_enabled?
......
...@@ -34,8 +34,8 @@ module Gitlab ...@@ -34,8 +34,8 @@ module Gitlab
# reached. So all ancestors *lower* than the specified ancestor will be # reached. So all ancestors *lower* than the specified ancestor will be
# included. # included.
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def ancestors(upto: nil) def ancestors(upto: nil, hierarchy_order: nil)
base_and_ancestors(upto: upto).where.not(id: ancestors_base.select(:id)) base_and_ancestors(upto: upto, hierarchy_order: hierarchy_order).where.not(id: ancestors_base.select(:id))
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
...@@ -46,16 +46,17 @@ module Gitlab ...@@ -46,16 +46,17 @@ module Gitlab
# reached. So all ancestors *lower* than the specified acestor will be # reached. So all ancestors *lower* than the specified acestor will be
# included. # included.
# #
# Passing an `depth` with either `:asc` or `:desc` will cause the recursive # Passing a `hierarchy_order` with either `:asc` or `:desc` will cause the
# query to use a depth column to order by depth (`:asc` returns most nested # recursive query order from most nested group to root or from the root
# group to root; `desc` returns opposite order). We define 1 as the depth # ancestor to most nested group respectively. This uses a `depth` column
# for the base and increment as we go up each parent. # where `1` is defined as the depth for the base and increment as we go up
# each parent.
# rubocop: disable CodeReuse/ActiveRecord # 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? return ancestors_base unless Group.supports_nested_groups?
recursive_query = base_and_ancestors_cte(upto, depth).apply_to(model.all) recursive_query = base_and_ancestors_cte(upto, hierarchy_order).apply_to(model.all)
recursive_query = recursive_query.order(depth: depth) if depth recursive_query = recursive_query.order(depth: hierarchy_order) if hierarchy_order
read_only(recursive_query) read_only(recursive_query)
end end
...@@ -117,11 +118,12 @@ module Gitlab ...@@ -117,11 +118,12 @@ module Gitlab
private private
# rubocop: disable CodeReuse/ActiveRecord # 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) cte = SQL::RecursiveCTE.new(:base_and_ancestors)
depth_column = :depth
base_query = ancestors_base.except(:order) 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 cte << base_query
...@@ -131,7 +133,7 @@ module Gitlab ...@@ -131,7 +133,7 @@ module Gitlab
.where(groups_table[:id].eq(cte.table[:parent_id])) .where(groups_table[:id].eq(cte.table[:parent_id]))
.except(:order) .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 parent_query = parent_query.where(cte.table[:parent_id].not_eq(stop_id)) if stop_id
cte << parent_query cte << parent_query
......
...@@ -35,13 +35,25 @@ describe Gitlab::GroupHierarchy, :postgresql do ...@@ -35,13 +35,25 @@ describe Gitlab::GroupHierarchy, :postgresql do
.to raise_error(ActiveRecord::ReadOnlyRecord) .to raise_error(ActiveRecord::ReadOnlyRecord)
end end
context 'with depth option' do describe 'hierarchy_order option' do
let(:relation) 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 end
it 'orders by depth' do context ':asc' do
expect(relation.map(&:depth)).to eq([1, 2, 3]) 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 end
end end
......
...@@ -253,17 +253,21 @@ describe Clusters::Cluster do ...@@ -253,17 +253,21 @@ describe Clusters::Cluster do
end end
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_cluster) { create(:cluster, :provided_by_gcp, :group) }
let(:group) { group_cluster.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 context 'when project does not belong to this group' do
let(:project) { create(:project, group: create(:group)) } let(:project) { create(:project, group: create(:group)) }
it 'returns nothing' do it 'returns nothing' do
expect(subject).to be_empty is_expected.to be_empty
end end
end end
...@@ -271,11 +275,11 @@ describe Clusters::Cluster do ...@@ -271,11 +275,11 @@ describe Clusters::Cluster do
let(:project) { create(:project, group: group) } let(:project) { create(:project, group: group) }
it 'returns the group cluster' do it 'returns the group cluster' do
expect(subject).to eq([group_cluster]) is_expected.to eq([group_cluster])
end end
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_cluster) { create(:cluster, :provided_by_gcp, :group) }
let(:sub_group) { sub_group_cluster.group } let(:sub_group) { sub_group_cluster.group }
let(:project) { create(:project, group: sub_group) } let(:project) { create(:project, group: sub_group) }
...@@ -284,18 +288,26 @@ describe Clusters::Cluster do ...@@ -284,18 +288,26 @@ describe Clusters::Cluster do
sub_group.update!(parent: group) sub_group.update!(parent: group)
end end
it 'returns clusters in order, ascending the hierachy' do it 'returns clusters in order, descending the hierachy' do
expect(subject).to eq([group_cluster, sub_group_cluster]) 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
end end
context 'cluster_scope arg' do context 'scope chaining' do
let(:project) { create(:project, group: group) } 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 it 'returns nothing' do
expect(subject).to be_empty is_expected.to be_empty
end end
end end
end end
......
...@@ -2117,6 +2117,16 @@ describe Project do ...@@ -2117,6 +2117,16 @@ describe Project do
it 'includes ancestors upto but excluding the given ancestor' do it 'includes ancestors upto but excluding the given ancestor' do
expect(project.ancestors_upto(parent)).to contain_exactly(child2, child) expect(project.ancestors_upto(parent)).to contain_exactly(child2, child)
end 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 end
describe '#lfs_enabled?' do 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