diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index c77d1fd6019..e72fd8eb3a5 100644 --- a/app/finders/group_descendants_finder.rb +++ b/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 diff --git a/spec/controllers/groups/children_controller_spec.rb b/spec/controllers/groups/children_controller_spec.rb index cb1b460fc0e..22d3076c269 100644 --- a/spec/controllers/groups/children_controller_spec.rb +++ b/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}") } diff --git a/spec/finders/group_descendants_finder_spec.rb b/spec/finders/group_descendants_finder_spec.rb index 9cadd9a6000..375bcc9087e 100644 --- a/spec/finders/group_descendants_finder_spec.rb +++ b/spec/finders/group_descendants_finder_spec.rb @@ -73,7 +73,7 @@ describe GroupDescendantsFinder do end context 'with a filter' do - let(:params) { { filter: 'tes' } } + let(:params) { { filter: 'test' } } it 'includes only projects matching the filter' do _other_project = create(:project, namespace: group) @@ -198,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')