Merge branch 'bvl-preload-parents-after-pagination' into 'master'

Preload ancestors after pagination when filtering

Closes #40785

See merge request gitlab-org/gitlab-ce!20398

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