diff options
| -rw-r--r-- | app/finders/group_descendants_finder.rb | 63 | ||||
| -rw-r--r-- | app/models/concerns/group_descendant.rb | 21 | ||||
| -rw-r--r-- | app/models/project.rb | 4 | ||||
| -rw-r--r-- | app/serializers/group_child_entity.rb | 21 | ||||
| -rw-r--r-- | spec/controllers/groups_controller_spec.rb | 34 | ||||
| -rw-r--r-- | spec/finders/group_descendants_finder_spec.rb | 38 |
6 files changed, 110 insertions, 71 deletions
diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb index 33fb1bf0359..07178a026e8 100644 --- a/app/finders/group_descendants_finder.rb +++ b/app/finders/group_descendants_finder.rb @@ -13,12 +13,6 @@ class GroupDescendantsFinder Kaminari.paginate_array(children) end - # This allows us to fetch only the count without loading the objects. Unless - # the objects were already loaded. - def total_count - @total_count ||= subgroup_count + project_count - end - def subgroup_count @subgroup_count ||= if defined?(@children) children.count { |child| child.is_a?(Group) } @@ -38,7 +32,39 @@ class GroupDescendantsFinder private def children - @children ||= subgroups.with_route.includes(parent: [:route, :parent]) + projects.with_route.includes(namespace: [:route, :parent]) + return @children if @children + + projects_count = <<~PROJECTCOUNT + (SELECT COUNT(projects.id) AS preloaded_project_count + FROM projects WHERE projects.namespace_id = namespaces.id) + PROJECTCOUNT + subgroup_count = <<~SUBGROUPCOUNT + (SELECT COUNT(children.id) AS preloaded_subgroup_count + FROM namespaces children + WHERE children.parent_id = namespaces.id) + SUBGROUPCOUNT + member_count = <<~MEMBERCOUNT + (SELECT COUNT(members.user_id) AS preloaded_member_count + FROM members + WHERE members.source_type = 'Namespace' + AND members.source_id = namespaces.id + AND members.requested_at IS NULL) + MEMBERCOUNT + group_selects = [ + 'namespaces.*', + projects_count, + subgroup_count, + member_count + ] + + subgroups_with_counts = subgroups.with_route.select(group_selects) + + if params[:filter] + ancestors_for_project_search = ancestors_for_groups(Group.where(id: projects_matching_filter.select(:namespace_id))) + subgroups_with_counts = ancestors_for_project_search.with_route.select(group_selects) | subgroups_with_counts + end + + @children = subgroups_with_counts + projects.preload(:route) end def direct_child_groups @@ -48,11 +74,19 @@ class GroupDescendantsFinder end def all_descendant_groups - Gitlab::GroupHierarchy.new(Group.where(id: parent_group)).base_and_descendants + Gitlab::GroupHierarchy.new(Group.where(id: parent_group)) + .base_and_descendants end def subgroups_matching_filter - all_descendant_groups.where.not(id: parent_group).search(params[:filter]) + all_descendant_groups + .where.not(id: parent_group) + .search(params[:filter]) + end + + def ancestors_for_groups(base_for_ancestors) + Gitlab::GroupHierarchy.new(base_for_ancestors) + .base_and_ancestors.where.not(id: parent_group) end def subgroups @@ -62,20 +96,23 @@ 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] - subgroups_matching_filter + ancestors_for_groups(subgroups_matching_filter) else direct_child_groups end groups.sort(params[:sort]) end + def projects_for_user + Project.public_or_visible_to_user(current_user).non_archived + end + def direct_child_projects - GroupProjectsFinder.new(group: parent_group, params: params, current_user: current_user).execute + projects_for_user.where(namespace: parent_group) end def projects_matching_filter - ProjectsFinder.new(current_user: current_user, params: params).execute - .search(params[:filter]) + projects_for_user.search(params[:filter]) .where(namespace: all_descendant_groups) end diff --git a/app/models/concerns/group_descendant.rb b/app/models/concerns/group_descendant.rb index 528fcaa1917..f850f71d661 100644 --- a/app/models/concerns/group_descendant.rb +++ b/app/models/concerns/group_descendant.rb @@ -1,16 +1,19 @@ module GroupDescendant - def hierarchy(hierarchy_top = nil) - expand_hierarchy_for_child(self, self, hierarchy_top) + def hierarchy(hierarchy_top = nil, preloaded = []) + expand_hierarchy_for_child(self, self, hierarchy_top, preloaded) end - def expand_hierarchy_for_child(child, hierarchy, hierarchy_top) - if child.parent.nil? && hierarchy_top.present? + def expand_hierarchy_for_child(child, hierarchy, hierarchy_top, preloaded = []) + parent = preloaded.detect { |possible_parent| possible_parent.is_a?(Group) && possible_parent.id == child.parent_id } + parent ||= child.parent + + if parent.nil? && hierarchy_top.present? raise ArgumentError.new('specified base is not part of the tree') end - if child.parent && child.parent != hierarchy_top - expand_hierarchy_for_child(child.parent, - { child.parent => hierarchy }, + if parent && parent != hierarchy_top + expand_hierarchy_for_child(parent, + { parent => hierarchy }, hierarchy_top) else hierarchy @@ -30,10 +33,10 @@ module GroupDescendant end first_descendant, *other_descendants = descendants - merged = first_descendant.hierarchy(hierarchy_top) + merged = first_descendant.hierarchy(hierarchy_top, descendants) other_descendants.each do |descendant| - next_descendant = descendant.hierarchy(hierarchy_top) + next_descendant = descendant.hierarchy(hierarchy_top, descendants) merged = merge_hash_tree(merged, next_descendant) end diff --git a/app/models/project.rb b/app/models/project.rb index ad0b717308a..110f1ba04a4 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1525,6 +1525,10 @@ class Project < ActiveRecord::Base namespace end + def parent_id + namespace_id + end + def parent_changed? namespace_id_changed? end diff --git a/app/serializers/group_child_entity.rb b/app/serializers/group_child_entity.rb index 8a2624c3d32..91e26272355 100644 --- a/app/serializers/group_child_entity.rb +++ b/app/serializers/group_child_entity.rb @@ -60,15 +60,23 @@ class GroupChildEntity < Grape::Entity end def children_count - @children_count ||= children_finder.total_count + @children_count ||= project_count + subgroup_count end def project_count - @project_count ||= children_finder.project_count + @project_count ||= if object.respond_to?(:preloaded_project_count) + object.preloaded_project_count + else + children_finder.project_count + end end def subgroup_count - @subgroup_count ||= children_finder.subgroup_count + @subgroup_count ||= if object.respond_to?(:preloaded_subgroup_count) + object.preloaded_subgroup_count + else + children_finder.subgroup_count + end end def leave_path @@ -88,6 +96,11 @@ class GroupChildEntity < Grape::Entity end def number_users_with_delimiter - number_with_delimiter(object.users.count) + member_count = if object.respond_to?(:preloaded_member_count) + object.preloaded_member_count + else + object.users.count + end + number_with_delimiter(member_count) end end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index befd346596f..84207144036 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -303,10 +303,12 @@ describe GroupsController do end context 'queries per rendered element', :request_store do - # The expected extra queries for the rendered group are: + # We need to make sure the following counts are preloaded + # otherwise they will cause an extra query # 1. Count of visible projects in the element # 2. Count of visible subgroups in the element - let(:expected_queries_per_group) { 2 } + # 3. Count of members of a group + let(:expected_queries_per_group) { 0 } let(:expected_queries_per_project) { 0 } def get_list @@ -329,13 +331,9 @@ describe GroupsController do end context 'when rendering hierarchies' do - # Extra queries per group when rendering a hierarchy: - # The route and the namespace are `included` for all matched elements - # But the parent's above those are not, so there's 2 extra queries per - # nested level: - # 1. Loading the parent that wasn't loaded yet - # 2. Loading the route for that parent. - let(:extra_queries_per_nested_level) { expected_queries_per_group + 2 } + # When loading hierarchies we load the all the ancestors for matched projects + # in 1 separate query + let(:extra_queries_for_hierarchies) { 1 } def get_filtered_list get :children, id: group.to_param, filter: 'filter', format: :json @@ -348,7 +346,7 @@ describe GroupsController do matched_group.update!(parent: public_subgroup) - expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level) + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies) end it 'queries the expected amount when a new group match is added' do @@ -357,8 +355,9 @@ describe GroupsController do control = ActiveRecord::QueryRecorder.new { get_filtered_list } create(:group, :public, parent: public_subgroup, name: 'filterme2') + create(:group, :public, parent: public_subgroup, name: 'filterme3') - expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level) + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies) end it 'queries the expected amount when nested rows are increased for a project' do @@ -368,18 +367,7 @@ describe GroupsController do matched_project.update!(namespace: public_subgroup) - expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level) - end - - it 'queries the expected amount when a new project match is added' do - create(:project, :public, namespace: public_subgroup, name: 'filterme') - - control = ActiveRecord::QueryRecorder.new { get_filtered_list } - - nested_group = create(:group, :public, parent: group) - create(:project, :public, namespace: nested_group, name: 'filterme2') - - expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_per_nested_level) + expect { get_filtered_list }.not_to exceed_query_limit(control).with_threshold(extra_queries_for_hierarchies) end end end diff --git a/spec/finders/group_descendants_finder_spec.rb b/spec/finders/group_descendants_finder_spec.rb index 77401ba09a2..09a773aaf68 100644 --- a/spec/finders/group_descendants_finder_spec.rb +++ b/spec/finders/group_descendants_finder_spec.rb @@ -46,6 +46,18 @@ describe GroupDescendantsFinder do expect(finder.execute).to contain_exactly(subgroup, project) end + it 'includes the preloaded counts for groups' do + create(:group, parent: subgroup) + create(:project, namespace: subgroup) + subgroup.add_developer(create(:user)) + + found_group = finder.execute.detect { |child| child.is_a?(Group) } + + expect(found_group.preloaded_project_count).to eq(1) + expect(found_group.preloaded_subgroup_count).to eq(1) + expect(found_group.preloaded_member_count).to eq(1) + end + context 'with a filter' do let(:params) { { filter: 'test' } } @@ -57,16 +69,16 @@ describe GroupDescendantsFinder do end context 'with matching children' do - it 'includes a group that has a subgroup matching the query' do + it 'includes a group that has a subgroup matching the query and its parent' do matching_subgroup = create(:group, name: 'testgroup', parent: subgroup) - expect(finder.execute).to contain_exactly(matching_subgroup) + expect(finder.execute).to contain_exactly(subgroup, matching_subgroup) end - it 'includes a group that has a project matching the query' do + it 'includes the parent of a matching project' do matching_project = create(:project, namespace: subgroup, name: 'Testproject') - expect(finder.execute).to contain_exactly(matching_project) + expect(finder.execute).to contain_exactly(subgroup, matching_project) end it 'does not include the parent itself' do @@ -77,23 +89,5 @@ describe GroupDescendantsFinder do end end end - - describe '#total_count' do - it 'counts the array children were already loaded' do - finder.instance_variable_set(:@children, [build(:project)]) - - expect(finder).not_to receive(:subgroups) - expect(finder).not_to receive(:projects) - - expect(finder.total_count).to eq(1) - end - - it 'performs a count without loading children when they are not loaded yet' do - expect(finder).to receive(:subgroups).and_call_original - expect(finder).to receive(:projects).and_call_original - - expect(finder.total_count).to eq(2) - end - end end end |