Force parents to be preloaded for building a hierarchy

app/models/concerns/group_descendant.rb

@@ -22,16 +22,22 @@ module GroupDescendant
   private
 
   def ancestors_upto(hierarchy_top = nil)
-    if hierarchy_top
-      Gitlab::GroupHierarchy.new(Group.where(id: hierarchy_top)).base_and_descendants
+    if self.is_a?(Group)
+      Gitlab::GroupHierarchy.new(Group.where(id: id))
+        .ancestors(upto: hierarchy_top)
     else
-      Gitlab::GroupHierarchy.new(Group.where(id: self)).all_groups
+      Gitlab::GroupHierarchy.new(Group.where(id: parent_id))
+        .base_and_ancestors(upto: hierarchy_top)
     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
+    parent = hierarchy_top if hierarchy_top && child.parent_id == hierarchy_top.id
+    parent ||= preloaded.detect { |possible_parent| possible_parent.is_a?(Group) && possible_parent.id == child.parent_id }
+
+    if parent.nil? && !child.parent_id.nil?
+      raise ArgumentError.new('parent was not preloaded')
+    end
 
     if parent.nil? && hierarchy_top.present?
       raise ArgumentError.new('specified top is not part of the tree')

spec/models/concerns/group_descendant_spec.rb

@@ -5,6 +5,10 @@ describe GroupDescendant, :nested_groups do
   let(:subgroup) { create(:group, parent: parent) }
   let(:subsub_group) { create(:group, parent: subgroup) }
 
+  def all_preloaded_groups(*groups)
+    groups + [parent, subgroup, subsub_group]
+  end
+
   context 'for a group' do
     describe '#hierarchy' do
       it 'only queries once for the ancestors' do
@@ -19,9 +23,8 @@ describe GroupDescendant, :nested_groups do
       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)
+        recorder = ActiveRecord::QueryRecorder.new { test_group.hierarchy(subgroup) }
+        expect(recorder.count).to eq(1)
       end
 
       it 'builds a hierarchy for a group' do
@@ -37,7 +40,8 @@ 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(build_stubbed(:group)) }.to raise_error('specified top 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
 
@@ -46,7 +50,7 @@ describe GroupDescendant, :nested_groups do
         other_subgroup = create(:group, parent: parent)
         other_subsub_group = create(:group, parent: subgroup)
 
-        groups = [other_subgroup, subsub_group, other_subsub_group]
+        groups = all_preloaded_groups(other_subgroup, subsub_group, other_subsub_group)
 
         expected_hierarchy = { parent => [other_subgroup, { subgroup => [subsub_group, other_subsub_group] }] }
 
@@ -58,9 +62,9 @@ describe GroupDescendant, :nested_groups do
         other_subsub_group = create(:group, parent: subgroup)
 
         groups = [other_subgroup, subsub_group, other_subsub_group]
+        groups << subgroup # Add the parent as if it was preloaded
 
         expected_hierarchy = [other_subgroup, { subgroup => [subsub_group, other_subsub_group] }]
-
         expect(described_class.build_hierarchy(groups, parent)).to eq(expected_hierarchy)
       end
 
@@ -69,11 +73,16 @@ describe GroupDescendant, :nested_groups do
         other_subgroup2 = create(:group, parent: parent)
         other_subsub_group = create(:group, parent: other_subgroup)
 
-        groups = [subsub_group, other_subgroup2, other_subsub_group]
+        groups = all_preloaded_groups(subsub_group, other_subgroup2, other_subsub_group, other_subgroup)
         expected_hierarchy = { parent => [{ subgroup => subsub_group }, other_subgroup2, { other_subgroup => other_subsub_group }] }
 
         expect(described_class.build_hierarchy(groups)).to eq(expected_hierarchy)
       end
+
+      it 'raises an error if not all elements were preloaded' do
+        expect { described_class.build_hierarchy([subsub_group]) }
+          .to raise_error('parent was not preloaded')
+      end
     end
   end
 
@@ -81,7 +90,7 @@ describe GroupDescendant, :nested_groups do
     let(:project) { create(:project, namespace: subsub_group) }
 
     describe '#hierarchy' do
-      it 'builds a hierarchy for a group' do
+      it 'builds a hierarchy for a project' do
         expected_hierarchy = { parent => { subgroup => { subsub_group => project } } }
 
         expect(project.hierarchy).to eq(expected_hierarchy)
@@ -92,10 +101,6 @@ describe GroupDescendant, :nested_groups do
 
         expect(project.hierarchy(subgroup)).to eq(expected_hierarchy)
       end
-
-      it 'raises an error if specifying a base that is not part of the tree' do
-        expect { project.hierarchy(build_stubbed(:group)) }.to raise_error('specified top is not part of the tree')
-      end
     end
 
     describe '.build_hierarchy' do
@@ -103,7 +108,7 @@ describe GroupDescendant, :nested_groups do
         other_project = create(:project, namespace: parent)
         other_subgroup_project = create(:project, namespace: subgroup)
 
-        elements = [other_project, subsub_group, other_subgroup_project]
+        elements = all_preloaded_groups(other_project, subsub_group, other_subgroup_project)
 
         expected_hierarchy = { parent => [other_project, { subgroup => [subsub_group, other_subgroup_project] }] }
 
@@ -115,6 +120,7 @@ describe GroupDescendant, :nested_groups do
         other_subgroup_project = create(:project, namespace: subgroup)
 
         elements = [other_project, subsub_group, other_subgroup_project]
+        elements << subgroup # Added as if it was preloaded
 
         expected_hierarchy = [other_project, { subgroup => [subsub_group, other_subgroup_project] }]
 
@@ -136,7 +142,9 @@ describe GroupDescendant, :nested_groups do
         other_subgroup = create(:group, parent: parent)
         other_subproject = create(:project, namespace: other_subgroup)
 
-        projects = [project, subsubsub_project, sub_project, other_subproject, subsub_project]
+        elements = [project, subsubsub_project, sub_project, other_subproject, subsub_project]
+        # Add parent groups as if they were preloaded
+        elements += [other_subgroup, subsubsub_group, subsub_group, subgroup]
 
         expected_hierarchy = [
           project,
@@ -149,7 +157,7 @@ describe GroupDescendant, :nested_groups do
           { other_subgroup => other_subproject }
         ]
 
-        actual_hierarchy = described_class.build_hierarchy(projects, parent)
+        actual_hierarchy = described_class.build_hierarchy(elements, parent)
 
         expect(actual_hierarchy).to eq(expected_hierarchy)
       end

spec/serializers/group_child_serializer_spec.rb

@@ -27,7 +27,7 @@ describe GroupChildSerializer do
         subgroup = create(:group, parent: parent)
         subsub_group = create(:group, parent: subgroup)
 
-        json = serializer.represent(subsub_group)
+        json = serializer.represent([subgroup, subsub_group]).first
         subsub_group_json = json[:children].first
 
         expect(json[:id]).to eq(subgroup.id)
@@ -41,7 +41,7 @@ describe GroupChildSerializer do
         subgroup2 = create(:group, parent: parent)
         subsub_group2 = create(:group, parent: subgroup2)
 
-        json = serializer.represent([subsub_group1, subsub_group2])
+        json = serializer.represent([subgroup1, subsub_group1, subgroup1, subgroup2])
         subgroup1_json = json.first
         subsub_group1_json = subgroup1_json[:children].first
 
@@ -58,7 +58,7 @@ describe GroupChildSerializer do
         it 'can render a tree' do
           subgroup = create(:group, parent: parent)
 
-          json = serializer.represent([subgroup])
+          json = serializer.represent([parent, subgroup])
           parent_json = json.first
 
           expect(parent_json[:id]).to eq(parent.id)
@@ -89,7 +89,7 @@ describe GroupChildSerializer do
           subgroup2 = create(:group, parent: parent)
           project2 = create(:project, namespace: subgroup2)
 
-          json = serializer.represent([project1, project2])
+          json = serializer.represent([project1, project2, subgroup1, subgroup2])
           project1_json, project2_json = json.map { |group_json| group_json[:children].first }
 
           expect(json.size).to eq(2)