Merge branch 'bvl-parent-preloading' into 'master'

Fix filtering projects & groups on group pages

Closes #40785

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

app/controllers/concerns/group_tree.rb

@@ -2,7 +2,11 @@ module GroupTree
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def render_group_tree(groups)
     @groups = if params[:filter].present?
-                Gitlab::GroupHierarchy.new(groups.search(params[:filter]))
+                # 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

app/finders/group_descendants_finder.rb

@@ -27,12 +27,16 @@ class GroupDescendantsFinder
   end
 
   def execute
-    # The children array might be extended with the ancestors of projects when
-    # filtering. In that case, take the maximum so the array does not get limited
-    # Otherwise, allow paginating through all results
+    # The children array might be extended with the ancestors of projects and
+    # subgroups when filtering. In that case, take the maximum so the array does
+    # not get limited otherwise, allow paginating through all results.
     #
     all_required_elements = children
-    all_required_elements |= ancestors_for_projects if params[:filter]
+    if params[:filter]
+      all_required_elements |= ancestors_of_filtered_subgroups
+      all_required_elements |= ancestors_of_filtered_projects
+    end
+
     total_count = [all_required_elements.size, paginator.total_count].max
 
     Kaminari.paginate_array(all_required_elements, total_count: total_count)
@@ -49,8 +53,11 @@ class GroupDescendantsFinder
   end
 
   def paginator
-    @paginator ||= Gitlab::MultiCollectionPaginator.new(subgroups, projects,
-                                                        per_page: params[:per_page])
+    @paginator ||= Gitlab::MultiCollectionPaginator.new(
+      subgroups,
+      projects.with_route,
+      per_page: params[:per_page]
+    )
   end
 
   def direct_child_groups
@@ -94,15 +101,21 @@ class GroupDescendantsFinder
   #
   # So when searching 'project', on the 'subgroup' page we want to preload
   # 'nested-group' but not 'subgroup' or 'root'
-  def ancestors_for_groups(base_for_ancestors)
-    Gitlab::GroupHierarchy.new(base_for_ancestors)
+  def ancestors_of_groups(base_for_ancestors)
+    group_ids = base_for_ancestors.except(:select, :sort).select(:id)
+    Gitlab::GroupHierarchy.new(Group.where(id: group_ids))
       .base_and_ancestors(upto: parent_group.id)
   end
 
-  def ancestors_for_projects
+  def ancestors_of_filtered_projects
     projects_to_load_ancestors_of = projects.where.not(namespace: parent_group)
     groups_to_load_ancestors_of = Group.where(id: projects_to_load_ancestors_of.select(:namespace_id))
-    ancestors_for_groups(groups_to_load_ancestors_of)
+    ancestors_of_groups(groups_to_load_ancestors_of)
+      .with_selects_for_list(archived: params[:archived])
+  end
+
+  def ancestors_of_filtered_subgroups
+    ancestors_of_groups(subgroups)
       .with_selects_for_list(archived: params[:archived])
   end
 
@@ -112,7 +125,7 @@ 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]
-               ancestors_for_groups(subgroups_matching_filter)
+               subgroups_matching_filter
              else
                direct_child_groups
              end
@@ -121,8 +134,10 @@ class GroupDescendantsFinder
   end
 
   def direct_child_projects
-    GroupProjectsFinder.new(group: parent_group, current_user: current_user, params: params)
-      .execute
+    GroupProjectsFinder.new(group: parent_group,
+                            current_user: current_user,
+                            options: { only_owned: true },
+                            params: params).execute
   end
 
   # Finds all projects nested under `parent_group` or any of its descendant

changelogs/unreleased/bvl-parent-preloading.yml

@@ -0,0 +1,5 @@
+---
+title: Fix issues when rendering groups and their children
+merge_request: 16584
+author:
+type: fixed

spec/controllers/dashboard/groups_controller_spec.rb

@@ -20,4 +20,24 @@ describe Dashboard::GroupsController do
 
     expect(assigns(:groups)).to contain_exactly(member_of_group)
   end
+
+  context 'when rendering an expanded hierarchy with public groups you are not a member of', :nested_groups do
+    let!(:top_level_result) { create(:group, name: 'chef-top') }
+    let!(:top_level_a) { create(:group, name: 'top-a') }
+    let!(:sub_level_result_a) { create(:group, name: 'chef-sub-a', parent: top_level_a) }
+    let!(:other_group) { create(:group, name: 'other') }
+
+    before do
+      top_level_result.add_master(user)
+      top_level_a.add_master(user)
+    end
+
+    it 'renders only groups the user is a member of when searching hierarchy correctly' do
+      get :index, filter: 'chef', format: :json
+
+      expect(response).to have_gitlab_http_status(200)
+      all_groups = [top_level_result, top_level_a, sub_level_result_a]
+      expect(assigns(:groups)).to contain_exactly(*all_groups)
+    end
+  end
 end

spec/controllers/groups/children_controller_spec.rb

@@ -160,6 +160,30 @@ describe Groups::ChildrenController do
           expect(json_response).to eq([])
         end
 
+        it 'succeeds if multiple pages contain matching subgroups' do
+          create(:group, parent: group, name: 'subgroup-filter-1')
+          create(:group, parent: group, name: 'subgroup-filter-2')
+
+          # Creating the group-to-nest first so it would be loaded into the
+          # relation first before it's parents, this is what would cause the
+          # crash in: https://gitlab.com/gitlab-org/gitlab-ce/issues/40785.
+          #
+          # If we create the parent groups first, those would be loaded into the
+          # collection first, and the pagination would cut off the actual search
+          # result. In this case the hierarchy can be rendered without crashing,
+          # it's just incomplete.
+          group_to_nest = create(:group, parent: group, name: 'subsubgroup-filter-3')
+          subgroup = create(:group, parent: group)
+          3.times do |i|
+            subgroup = create(:group, parent: subgroup)
+          end
+          group_to_nest.update!(parent: subgroup)
+
+          get :index, group_id: group.to_param, filter: 'filter', per_page: 3, format: :json
+
+          expect(response).to have_gitlab_http_status(200)
+        end
+
         it 'includes pagination headers' do
           2.times { |i| create(:group, :public, parent: public_subgroup, name: "filterme#{i}") }
 

spec/finders/group_descendants_finder_spec.rb

@@ -35,6 +35,15 @@ describe GroupDescendantsFinder do
       expect(finder.execute).to contain_exactly(project)
     end
 
+    it 'does not include projects shared with the group' do
+      project = create(:project, namespace: group)
+      other_project = create(:project)
+      other_project.project_group_links.create(group: group,
+                                               group_access: ProjectGroupLink::MASTER)
+
+      expect(finder.execute).to contain_exactly(project)
+    end
+
     context 'when archived is `true`' do
       let(:params) { { archived: 'true' } }
 
@@ -189,6 +198,17 @@ describe GroupDescendantsFinder do
             expect(finder.execute).to contain_exactly(subgroup, matching_project)
           end
 
+          context 'with a small page size' do
+            let(:params) { { filter: 'test', per_page: 1 } }
+
+            it 'contains all the ancestors of a matching subgroup regardless the page size' do
+              subgroup = create(:group, :private, parent: group)
+              matching = create(:group, :private, name: 'testgroup', parent: subgroup)
+
+              expect(finder.execute).to contain_exactly(subgroup, matching)
+            end
+          end
+
           it 'does not include the parent itself' do
             group.update!(name: 'test')