From f85440e63c2eba453e09a5599f0d3e0491a037f1 Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Mon, 3 Dec 2018 10:22:33 +1300 Subject: [PATCH] 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 --- app/models/clusters/cluster.rb | 8 ++---- app/models/concerns/deployment_platform.rb | 4 +-- app/models/namespace.rb | 4 +-- app/models/project.rb | 4 +-- lib/gitlab/group_hierarchy.rb | 26 ++++++++++-------- spec/lib/gitlab/group_hierarchy_spec.rb | 20 +++++++++++--- spec/models/clusters/cluster_spec.rb | 32 +++++++++++++++------- spec/models/project_spec.rb | 10 +++++++ 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