Commit 167fd713 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Always preload all elements in a hierarchy to avoid extra queries.

parent ef043063
module GroupDescendant 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) expand_hierarchy_for_child(self, self, hierarchy_top, preloaded)
end end
...@@ -24,12 +25,20 @@ module GroupDescendant ...@@ -24,12 +25,20 @@ module GroupDescendant
private 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 = preloaded.detect { |possible_parent| possible_parent.is_a?(Group) && possible_parent.id == child.parent_id }
parent ||= child.parent parent ||= child.parent
if parent.nil? && hierarchy_top.present? 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 end
if parent && parent != hierarchy_top if parent && parent != hierarchy_top
......
...@@ -7,6 +7,23 @@ describe GroupDescendant, :nested_groups do ...@@ -7,6 +7,23 @@ describe GroupDescendant, :nested_groups do
context 'for a group' do context 'for a group' do
describe '#hierarchy' 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 it 'builds a hierarchy for a group' do
expected_hierarchy = { parent => { subgroup => subsub_group } } expected_hierarchy = { parent => { subgroup => subsub_group } }
...@@ -20,7 +37,7 @@ describe GroupDescendant, :nested_groups do ...@@ -20,7 +37,7 @@ describe GroupDescendant, :nested_groups do
end end
it 'raises an error if specifying a base that is not part of the tree' do 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
end end
...@@ -77,7 +94,7 @@ describe GroupDescendant, :nested_groups do ...@@ -77,7 +94,7 @@ describe GroupDescendant, :nested_groups do
end end
it 'raises an error if specifying a base that is not part of the tree' do 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
end end
......
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