From 93e17dc3e96fd0c0f24cf5cbfff2f26529448f52 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 22 Jan 2018 16:41:19 +0000 Subject: 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 | 6 +++- app/finders/group_descendants_finder.rb | 41 +++++++++++++++------- changelogs/unreleased/bvl-parent-preloading.yml | 5 +++ .../dashboard/groups_controller_spec.rb | 20 +++++++++++ .../controllers/groups/children_controller_spec.rb | 24 +++++++++++++ spec/finders/group_descendants_finder_spec.rb | 20 +++++++++++ 6 files changed, 102 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/bvl-parent-preloading.yml diff --git a/app/controllers/concerns/group_tree.rb b/app/controllers/concerns/group_tree.rb index b10147835f3..6d564d4642c 100644 --- a/app/controllers/concerns/group_tree.rb +++ b/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 diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index 1a5f6063437..9e6cdd3c5a5 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 @@ -93,15 +100,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 @@ -111,7 +124,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 @@ -119,8 +132,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 diff --git a/changelogs/unreleased/bvl-parent-preloading.yml b/changelogs/unreleased/bvl-parent-preloading.yml new file mode 100644 index 00000000000..97c7bbb2a2a --- /dev/null +++ b/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 diff --git a/spec/controllers/dashboard/groups_controller_spec.rb b/spec/controllers/dashboard/groups_controller_spec.rb index fb9d3efbac0..7f2eaf95165 100644 --- a/spec/controllers/dashboard/groups_controller_spec.rb +++ b/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 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 ae050f36b4a..375bcc9087e 100644 --- a/spec/finders/group_descendants_finder_spec.rb +++ b/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') -- cgit v1.2.1