Minimize the number of queries by preloading counts and ancestors

By preloading the count of members, projects and subgroups of a group,
we don't need to query them later.

We also preload the entire hierarchy for a search result and include
the counts so we don't need to query for them again

app/finders/group_descendants_finder.rb

@@ -13,12 +13,6 @@ class GroupDescendantsFinder
     Kaminari.paginate_array(children)
   end
-  # This allows us to fetch only the count without loading the objects. Unless
-  # the objects were already loaded.
-  def total_count
-    @total_count ||= subgroup_count + project_count
-  end
-
   def subgroup_count
     @subgroup_count ||= if defined?(@children)
                           children.count { |child| child.is_a?(Group) }
@@ -38,7 +32,39 @@ class GroupDescendantsFinder
   private
   def children
-    @children ||= subgroups.with_route.includes(parent: [:route, :parent]) + projects.with_route.includes(namespace: [:route, :parent])
+    return @children if @children
+
+    projects_count = <<~PROJECTCOUNT
+                   (SELECT COUNT(projects.id) AS preloaded_project_count
+                    FROM projects WHERE projects.namespace_id = namespaces.id)
+                   PROJECTCOUNT
+    subgroup_count = <<~SUBGROUPCOUNT
+                     (SELECT COUNT(children.id) AS preloaded_subgroup_count
+                      FROM namespaces children
+                      WHERE children.parent_id = namespaces.id)
+                     SUBGROUPCOUNT
+    member_count = <<~MEMBERCOUNT
+                   (SELECT COUNT(members.user_id) AS preloaded_member_count
+                    FROM members
+                    WHERE members.source_type = 'Namespace'
+                    AND members.source_id = namespaces.id
+                    AND members.requested_at IS NULL)
+                   MEMBERCOUNT
+    group_selects = [
+      'namespaces.*',
+      projects_count,
+      subgroup_count,
+      member_count
+    ]
+
+    subgroups_with_counts = subgroups.with_route.select(group_selects)
+
+    if params[:filter]
+      ancestors_for_project_search = ancestors_for_groups(Group.where(id: projects_matching_filter.select(:namespace_id)))
+      subgroups_with_counts = ancestors_for_project_search.with_route.select(group_selects) | subgroups_with_counts
+    end
+
+    @children = subgroups_with_counts + projects.preload(:route)
   end
   def direct_child_groups
@@ -48,11 +74,19 @@ class GroupDescendantsFinder
   end
   def all_descendant_groups
-    Gitlab::GroupHierarchy.new(Group.where(id: parent_group)).base_and_descendants
+    Gitlab::GroupHierarchy.new(Group.where(id: parent_group))
+      .base_and_descendants
   end
   def subgroups_matching_filter
-    all_descendant_groups.where.not(id: parent_group).search(params[:filter])
+    all_descendant_groups
+      .where.not(id: parent_group)
+      .search(params[:filter])
+  end
+
+  def ancestors_for_groups(base_for_ancestors)
+    Gitlab::GroupHierarchy.new(base_for_ancestors)
+      .base_and_ancestors.where.not(id: parent_group)
   end
   def subgroups
@@ -62,20 +96,23 @@ class GroupDescendantsFinder
     # When filtering subgroups, we want to find all matches withing the tree of
     # descendants to show to the user
     groups = if params[:filter]
-               subgroups_matching_filter
+               ancestors_for_groups(subgroups_matching_filter)
              else
                direct_child_groups
              end
     groups.sort(params[:sort])
   end
+  def projects_for_user
+    Project.public_or_visible_to_user(current_user).non_archived
+  end
+
   def direct_child_projects
-    GroupProjectsFinder.new(group: parent_group, params: params, current_user: current_user).execute
+    projects_for_user.where(namespace: parent_group)
   end
   def projects_matching_filter
-    ProjectsFinder.new(current_user: current_user, params: params).execute
-      .search(params[:filter])
+    projects_for_user.search(params[:filter])
       .where(namespace: all_descendant_groups)
   end

app/models/concerns/group_descendant.rb

 module GroupDescendant
-  def hierarchy(hierarchy_top = nil)
-    expand_hierarchy_for_child(self, self, hierarchy_top)
+  def hierarchy(hierarchy_top = nil, preloaded = [])
+    expand_hierarchy_for_child(self, self, hierarchy_top, preloaded)
   end
-  def expand_hierarchy_for_child(child, hierarchy, hierarchy_top)
-    if child.parent.nil? && hierarchy_top.present?
+  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')
     end
-    if child.parent && child.parent != hierarchy_top
-      expand_hierarchy_for_child(child.parent,
-                                 { child.parent => hierarchy },
+    if parent && parent != hierarchy_top
+      expand_hierarchy_for_child(parent,
+                                 { parent => hierarchy },
                                  hierarchy_top)
     else
       hierarchy
@@ -30,10 +33,10 @@ module GroupDescendant
     end
     first_descendant, *other_descendants = descendants
-    merged = first_descendant.hierarchy(hierarchy_top)
+    merged = first_descendant.hierarchy(hierarchy_top, descendants)
     other_descendants.each do |descendant|
-      next_descendant = descendant.hierarchy(hierarchy_top)
+      next_descendant = descendant.hierarchy(hierarchy_top, descendants)
       merged = merge_hash_tree(merged, next_descendant)
     end

app/models/project.rb

@@ -1525,6 +1525,10 @@ class Project < ActiveRecord::Base
     namespace
   end
+  def parent_id
+    namespace_id
+  end
+
   def parent_changed?
     namespace_id_changed?
   end

app/serializers/group_child_entity.rb

@@ -60,15 +60,23 @@ class GroupChildEntity < Grape::Entity
   end
   def children_count
-    @children_count ||= children_finder.total_count
+    @children_count ||= project_count + subgroup_count
   end
   def project_count
-    @project_count ||= children_finder.project_count
+    @project_count ||= if object.respond_to?(:preloaded_project_count)
+                         object.preloaded_project_count
+                       else
+                         children_finder.project_count
+                       end
   end
   def subgroup_count
-    @subgroup_count ||= children_finder.subgroup_count
+    @subgroup_count ||= if object.respond_to?(:preloaded_subgroup_count)
+                          object.preloaded_subgroup_count
+                        else
+                          children_finder.subgroup_count
+                        end
   end
   def leave_path
@@ -88,6 +96,11 @@ class GroupChildEntity < Grape::Entity
   end
   def number_users_with_delimiter
-    number_with_delimiter(object.users.count)
+    member_count = if object.respond_to?(:preloaded_member_count)
+                     object.preloaded_member_count
+                   else
+                     object.users.count
+                   end
+    number_with_delimiter(member_count)
   end
 end

spec/controllers/groups_controller_spec.rb

@@ -303,10 +303,12 @@ describe GroupsController do
       end
       context 'queries per rendered element', :request_store do
-        # The expected extra queries for the rendered group are:
+        # We need to make sure the following counts are preloaded
+        # otherwise they will cause an extra query
         # 1. Count of visible projects in the element
         # 2. Count of visible subgroups in the element
-        let(:expected_queries_per_group) { 2 }
+        # 3. Count of members of a group
+        let(:expected_queries_per_group) { 0 }
         let(:expected_queries_per_project) { 0 }
         def get_list
@@ -329,13 +331,9 @@ describe GroupsController do
         end
         context 'when rendering hierarchies' do
-          # Extra queries per group when rendering a hierarchy:
-          # The route and the namespace are `included` for all matched elements
-          # But the parent's above those are not, so there's 2 extra queries per
-          # nested level:
-          # 1. Loading the parent that wasn't loaded yet
-          # 2. Loading the route for that parent.
-          let(:extra_queries_per_nested_level) { expected_queries_per_group + 2 }
+          # When loading hierarchies we load the all the ancestors for matched projects
+          # in 1 separate query
+          let(:extra_queries_for_hierarchies) { 1 }
           def get_filtered_list
             get :children, id: group.to_param, filter: 'filter', format: :json
@@ -348,7 +346,7 @@ describe GroupsController do
             matched_group.update!(parent: public_subgroup)
-            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level)
+            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies)
           end
           it 'queries the expected amount when a new group match is added' do
@@ -357,8 +355,9 @@ describe GroupsController do
             control = ActiveRecord::QueryRecorder.new { get_filtered_list }
             create(:group, :public, parent: public_subgroup, name: 'filterme2')
-            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level)
+            create(:group, :public, parent: public_subgroup, name: 'filterme3')
+            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies)
           end
           it 'queries the expected amount when nested rows are increased for a project' do
@@ -368,18 +367,7 @@ describe GroupsController do
             matched_project.update!(namespace: public_subgroup)
-            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level)
-          end
-
-          it 'queries the expected amount when a new project match is added' do
-            create(:project, :public, namespace: public_subgroup, name: 'filterme')
-
-            control = ActiveRecord::QueryRecorder.new { get_filtered_list }
-
-            nested_group = create(:group, :public, parent: group)
-            create(:project, :public, namespace: nested_group, name: 'filterme2')
-
-            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level)
+            expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies)
           end
         end
       end

spec/finders/group_descendants_finder_spec.rb

@@ -46,6 +46,18 @@ describe GroupDescendantsFinder do
         expect(finder.execute).to contain_exactly(subgroup, project)
       end
+      it 'includes the preloaded counts for groups' do
+        create(:group, parent: subgroup)
+        create(:project, namespace: subgroup)
+        subgroup.add_developer(create(:user))
+
+        found_group = finder.execute.detect { |child| child.is_a?(Group) }
+
+        expect(found_group.preloaded_project_count).to eq(1)
+        expect(found_group.preloaded_subgroup_count).to eq(1)
+        expect(found_group.preloaded_member_count).to eq(1)
+      end
+
       context 'with a filter' do
         let(:params) { { filter: 'test' } }
@@ -57,16 +69,16 @@ describe GroupDescendantsFinder do
         end
         context 'with matching children' do
-          it 'includes a group that has a subgroup matching the query' do
+          it 'includes a group that has a subgroup matching the query and its parent' do
             matching_subgroup = create(:group, name: 'testgroup', parent: subgroup)
-            expect(finder.execute).to contain_exactly(matching_subgroup)
+            expect(finder.execute).to contain_exactly(subgroup, matching_subgroup)
           end
-          it 'includes a group that has a project matching the query' do
+          it 'includes the parent of a matching project' do
             matching_project = create(:project, namespace: subgroup, name: 'Testproject')
-            expect(finder.execute).to contain_exactly(matching_project)
+            expect(finder.execute).to contain_exactly(subgroup, matching_project)
           end
           it 'does not include the parent itself' do
@@ -77,23 +89,5 @@ describe GroupDescendantsFinder do
         end
       end
     end
-
-    describe '#total_count' do
-      it 'counts the array children were already loaded' do
-        finder.instance_variable_set(:@children, [build(:project)])
-
-        expect(finder).not_to receive(:subgroups)
-        expect(finder).not_to receive(:projects)
-
-        expect(finder.total_count).to eq(1)
-      end
-
-      it 'performs a count without loading children when they are not loaded yet' do
-        expect(finder).to receive(:subgroups).and_call_original
-        expect(finder).to receive(:projects).and_call_original
-
-        expect(finder.total_count).to eq(2)
-      end
-    end
   end
 end