diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb index 7d8f051d2eb..f37d23e615e 100644 --- a/app/models/concerns/group_descendant.rb +++ b/app/models/concerns/group_descendant.rb @@ -1,5 +1,6 @@ module GroupDescendant - def hierarchy(hierarchy_top = nil, preloaded = []) + def hierarchy(hierarchy_top = nil, preloaded = nil) + preloaded ||= ancestors_upto(hierarchy_top) expand_hierarchy_for_child(self, self, hierarchy_top, preloaded) end @@ -24,12 +25,20 @@ module GroupDescendant private - def expand_hierarchy_for_child(child, hierarchy, hierarchy_top, preloaded = []) + def ancestors_upto(hierarchy_top = nil) + if hierarchy_top + Gitlab::GroupHierarchy.new(Group.where(id: hierarchy_top)).base_and_descendants + else + Gitlab::GroupHierarchy.new(Group.where(id: self)).all_groups + end + end + + def expand_hierarchy_for_child(child, hierarchy, hierarchy_top, preloaded) parent = preloaded.detect { |possible_parent| possible_parent.is_a?(Group) && possible_parent.id == child.parent_id } parent ||= child.parent if parent.nil? && hierarchy_top.present? - raise ArgumentError.new('specified base is not part of the tree') + raise ArgumentError.new('specified top is not part of the tree') end if parent && parent != hierarchy_top diff --git a/spec/models/concerns/group_descendant_spec.rb b/spec/models/concerns/group_descendant_spec.rb index f3a0c342d35..c17c8f2abec 100644 --- a/spec/models/concerns/group_descendant_spec.rb +++ b/spec/models/concerns/group_descendant_spec.rb @@ -7,6 +7,23 @@ describe GroupDescendant, :nested_groups do context 'for a group' do describe '#hierarchy' do + it 'only queries once for the ancestors' do + # make sure the subsub_group does not have anything cached + test_group = create(:group, parent: subsub_group).reload + + query_count = ActiveRecord::QueryRecorder.new { test_group.hierarchy }.count + + expect(query_count).to eq(1) + end + + it 'only queries once for the ancestors when a top is given' do + test_group = create(:group, parent: subsub_group).reload + + query_count = ActiveRecord::QueryRecorder.new { test_group.hierarchy(subgroup) }.count + + expect(query_count).to eq(1) + end + it 'builds a hierarchy for a group' do expected_hierarchy = { parent => { subgroup => subsub_group } } @@ -20,7 +37,7 @@ describe GroupDescendant, :nested_groups do end it 'raises an error if specifying a base that is not part of the tree' do - expect { subsub_group.hierarchy(double) }.to raise_error('specified base is not part of the tree') + expect { subsub_group.hierarchy(build_stubbed(:group)) }.to raise_error('specified top is not part of the tree') end end @@ -77,7 +94,7 @@ describe GroupDescendant, :nested_groups do end it 'raises an error if specifying a base that is not part of the tree' do - expect { project.hierarchy(double) }.to raise_error('specified base is not part of the tree') + expect { project.hierarchy(build_stubbed(:group)) }.to raise_error('specified top is not part of the tree') end end