Preload ancestors after pagination when filtering

We need to preload the ancestors of search results after applying
pagination limits. This way the search results itself are paginated,
but not the ancestors.

If we don't do this, we might not preload a parent group of a search
result as it has been cut off by pagination.

4 files changed, 44 insertions, 16 deletions

diff --git a/app/controllers/concerns/group_tree.rb b/app/controllers/concerns/group_tree.rb
index 56770a17406..6ec6897e707 100644
--- a/app/controllers/concerns/group_tree.rb
+++ b/app/controllers/concerns/group_tree.rb
@@ -1,21 +1,16 @@
 module GroupTree
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def render_group_tree(groups)
-    @groups = if params[:filter].present?
-                # We find the ancestors by ID of the search results here.
-                # Otherwise the ancestors would also have filters applied,
-                # which would cause them not to be preloaded.
-                group_ids = groups.search(params[:filter]).select(:id)
-                Gitlab::GroupHierarchy.new(Group.where(id: group_ids))
-                  .base_and_ancestors
-              else
-                # Only show root groups if no parent-id is given
-                groups.where(parent_id: params[:parent_id])
-              end
+    groups = groups.sort_by_attribute(@sort = params[:sort])
 
-    @groups = @groups.with_selects_for_list(archived: params[:archived])
-                .sort_by_attribute(@sort = params[:sort])
-                .page(params[:page])
+    groups = if params[:filter].present?
+               filtered_groups_with_ancestors(groups)
+             else
+               # If `params[:parent_id]` is `nil`, we will only show root-groups
+               groups.where(parent_id: params[:parent_id]).page(params[:page])
+             end
+
+    @groups = groups.with_selects_for_list(archived: params[:archived])
 
     respond_to do |format|
       format.html
@@ -28,4 +23,21 @@ module GroupTree
     end
     # rubocop:enable Gitlab/ModuleWithInstanceVariables
   end
+
+  def filtered_groups_with_ancestors(groups)
+    filtered_groups = groups.search(params[:filter]).page(params[:page])
+
+    if Group.supports_nested_groups?
+      # We find the ancestors by ID of the search results here.
+      # Otherwise the ancestors would also have filters applied,
+      # which would cause them not to be preloaded.
+      #
+      # Pagination needs to be applied before loading the ancestors to
+      # make sure ancestors are not cut off by pagination.
+      Gitlab::GroupHierarchy.new(Group.where(id: filtered_groups.select(:id)))
+        .base_and_ancestors
+    else
+      filtered_groups
+    end
+  end
 end
diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb
index 261ace57a17..5e9a95c3282 100644
--- a/app/models/concerns/group_descendant.rb
+++ b/app/models/concerns/group_descendant.rb
@@ -44,8 +44,8 @@ module GroupDescendant
         This error is not user facing, but causes a +1 query.
       MSG
       extras = {
-        parent: parent,
-        child: child,
+        parent: parent.inspect,
+        child: child.inspect,
         preloaded: preloaded.map(&:full_path)
       }
       issue_url = 'https://gitlab.com/gitlab-org/gitlab-ce/issues/40785'
diff --git a/changelogs/unreleased/bvl-preload-parents-after-pagination.yml b/changelogs/unreleased/bvl-preload-parents-after-pagination.yml
new file mode 100644
index 00000000000..ff3d4716d34
--- /dev/null
+++ b/changelogs/unreleased/bvl-preload-parents-after-pagination.yml
@@ -0,0 +1,5 @@
+---
+title: Reduce the number of queries when searching for groups
+merge_request: 20398
+author:
+type: performance
diff --git a/spec/controllers/concerns/group_tree_spec.rb b/spec/controllers/concerns/group_tree_spec.rb
index ba84fbf8564..503eb416962 100644
--- a/spec/controllers/concerns/group_tree_spec.rb
+++ b/spec/controllers/concerns/group_tree_spec.rb
@@ -63,6 +63,17 @@ describe GroupTree do
 
         expect(assigns(:groups)).to contain_exactly(parent, subgroup)
       end
+
+      it 'preloads parents regardless of pagination' do
+        allow(Kaminari.config).to receive(:default_per_page).and_return(1)
+        group = create(:group, :public)
+        subgroup = create(:group, :public, parent: group)
+        search_result = create(:group, :public, name: 'result', parent: subgroup)
+
+        get :index, filter: 'resu', format: :json
+
+        expect(assigns(:groups)).to contain_exactly(group, subgroup, search_result)
+      end
     end
 
     context 'json content' do