From d8591c7fca8c34bbe59166dd388a15a55a539d2d Mon Sep 17 00:00:00 2001
From: Lee Tickett <ltickett@gitlab.com>
Date: Fri, 2 Feb 2024 07:58:07 +0000
Subject: [PATCH] Fix group filter and pagination bug
Changelog: fixed
---
.rubocop_todo/layout/line_length.yml | 2 -
.rubocop_todo/rspec/feature_category.yml | 1 -
app/finders/group_descendants_finder.rb | 65 +++-
app/models/group.rb | 2 +-
.../select_ancestors_of_paginated_items.yml | 9 +
lib/gitlab/multi_collection_paginator.rb | 4 +-
spec/finders/group_descendants_finder_spec.rb | 301 +++++++++---------
spec/models/group_spec.rb | 7 +
spec/support/rspec_order_todo.yml | 1 -
9 files changed, 225 insertions(+), 167 deletions(-)
create mode 100644 config/feature_flags/gitlab_com_derisk/select_ancestors_of_paginated_items.yml
diff --git a/.rubocop_todo/layout/line_length.yml b/.rubocop_todo/layout/line_length.yml
index ffbf31c341420..ea079e765f41b 100644
--- a/.rubocop_todo/layout/line_length.yml
+++ b/.rubocop_todo/layout/line_length.yml
@@ -92,7 +92,6 @@ Layout/LineLength:
- 'app/finders/analytics/cycle_analytics/stage_finder.rb'
- 'app/finders/ci/runners_finder.rb'
- 'app/finders/clusters/agents/authorizations/ci_access/finder.rb'
- - 'app/finders/group_descendants_finder.rb'
- 'app/finders/group_projects_finder.rb'
- 'app/finders/issuable_finder.rb'
- 'app/finders/issuable_finder/params.rb'
@@ -3211,7 +3210,6 @@ Layout/LineLength:
- 'spec/finders/environments/environments_by_deployments_finder_spec.rb'
- 'spec/finders/environments/environments_finder_spec.rb'
- 'spec/finders/events_finder_spec.rb'
- - 'spec/finders/group_descendants_finder_spec.rb'
- 'spec/finders/group_members_finder_spec.rb'
- 'spec/finders/group_projects_finder_spec.rb'
- 'spec/finders/groups/user_groups_finder_spec.rb'
diff --git a/.rubocop_todo/rspec/feature_category.yml b/.rubocop_todo/rspec/feature_category.yml
index 3cc9f073136f8..dcd42b77f21f6 100644
--- a/.rubocop_todo/rspec/feature_category.yml
+++ b/.rubocop_todo/rspec/feature_category.yml
@@ -1743,7 +1743,6 @@ RSpec/FeatureCategory:
- 'spec/finders/feature_flags_user_lists_finder_spec.rb'
- 'spec/finders/fork_projects_finder_spec.rb'
- 'spec/finders/fork_targets_finder_spec.rb'
- - 'spec/finders/group_descendants_finder_spec.rb'
- 'spec/finders/group_projects_finder_spec.rb'
- 'spec/finders/groups/accepting_group_transfers_finder_spec.rb'
- 'spec/finders/groups/accepting_project_transfers_finder_spec.rb'
diff --git a/app/finders/group_descendants_finder.rb b/app/finders/group_descendants_finder.rb
index 3e31c7a2bb2f2..109081df79f66 100644
--- a/app/finders/group_descendants_finder.rb
+++ b/app/finders/group_descendants_finder.rb
@@ -20,6 +20,8 @@
# `non_archived` is passed to the `ProjectFinder`s if none
# was given.
class GroupDescendantsFinder
+ include Gitlab::Utils::StrongMemoize
+
attr_reader :current_user, :parent_group, :params
def initialize(parent_group:, current_user: nil, params: {})
@@ -44,14 +46,10 @@ def execute
Kaminari.paginate_array(all_required_elements, total_count: total_count)
end
- def has_children?
- projects.any? || subgroups.any?
- end
-
private
def children
- @children ||= paginator.paginate(params[:page])
+ @children ||= paginator.paginate(page)
end
def paginator
@@ -106,24 +104,39 @@ def subgroups_matching_filter
# 'nested-group' but not 'subgroup' or 'root'
# rubocop: disable CodeReuse/ActiveRecord
def ancestors_of_groups(base_for_ancestors)
- group_ids = base_for_ancestors.except(:select, :sort).select(:id)
- groups = Group.where(id: group_ids)
+ group_ids = if select_ancestors_of_paginated_items_feature_enabled?
+ base_for_ancestors
+ else
+ base_for_ancestors.except(:select, :sort).select(:id)
+ end
- groups.self_and_ancestors(upto: parent_group.id)
+ Group.where(id: group_ids).self_and_ancestors(upto: parent_group.id)
end
# rubocop: enable CodeReuse/ActiveRecord
# rubocop: disable CodeReuse/ActiveRecord
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))
+ # rubocop:disable Database/AvoidUsingPluckWithoutLimit -- Limit of 100 max per page is defined in kaminari config
+ groups_to_load_ancestors_of = if select_ancestors_of_paginated_items_feature_enabled?
+ paginated_projects_without_direct_descendents.pluck(:namespace_id)
+ else
+ projects_to_load_ancestors_of = projects.where.not(namespace: parent_group)
+ Group.where(id: projects_to_load_ancestors_of.select(:namespace_id))
+ end
+ # rubocop:enable Database/AvoidUsingPluckWithoutLimit
ancestors_of_groups(groups_to_load_ancestors_of)
.with_selects_for_list(archived: params[:archived])
end
# rubocop: enable CodeReuse/ActiveRecord
def ancestors_of_filtered_subgroups
- ancestors_of_groups(subgroups)
+ subgroups_to_find_ancestors = if select_ancestors_of_paginated_items_feature_enabled?
+ paginated_subgroups_without_direct_descendents
+ else
+ subgroups
+ end
+
+ ancestors_of_groups(subgroups_to_find_ancestors)
.with_selects_for_list(archived: params[:archived])
end
@@ -141,7 +154,8 @@ def subgroups
# rubocop: disable CodeReuse/Finder
def direct_child_projects
- GroupProjectsFinder.new(group: parent_group, current_user: current_user, params: params, options: { exclude_shared: true })
+ GroupProjectsFinder
+ .new(group: parent_group, current_user: current_user, params: params, options: { exclude_shared: true })
.execute
end
# rubocop: enable CodeReuse/Finder
@@ -177,9 +191,28 @@ def sort
params.fetch(:sort, 'name_asc')
end
- # rubocop: disable CodeReuse/ActiveRecord
- def hierarchy_for_parent
- @hierarchy ||= Gitlab::ObjectHierarchy.new(Group.where(id: parent_group.id))
+ def paginated_subgroups_without_direct_descendents
+ # We remove direct descendants (ie. item.parent_id == parent_group.id) as we already have their parent
+ # i.e. `parent_group`.
+ paginator
+ .paginated_first_collection(page)
+ .reject { |item| item.parent_id == parent_group.id }
end
- # rubocop: enable CodeReuse/ActiveRecord
+
+ def paginated_projects_without_direct_descendents
+ # We remove direct descendants (ie. item.namespace_id == parent_group.id) as we already have their parent
+ # i.e. `parent_group`.
+ paginator
+ .paginated_second_collection(page)
+ .reject { |item| item.namespace_id == parent_group.id }
+ end
+
+ def page
+ params[:page].to_i
+ end
+
+ def select_ancestors_of_paginated_items_feature_enabled?
+ Feature.enabled?(:select_ancestors_of_paginated_items, parent_group.root_ancestor, type: :gitlab_com_derisk)
+ end
+ strong_memoize_attr :select_ancestors_of_paginated_items_feature_enabled?
end
diff --git a/app/models/group.rb b/app/models/group.rb
index e44618f7a3329..a43815781f2c7 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -536,7 +536,7 @@ def member_owners_excluding_project_bots
owners = []
members_from_hiearchy.all_owners.non_invite.each_batch do |relation|
- owners += relation.preload(:user).load.reject do |member|
+ owners += relation.preload(:user, :source).load.reject do |member|
member.user.nil? || member.user.project_bot?
end
end
diff --git a/config/feature_flags/gitlab_com_derisk/select_ancestors_of_paginated_items.yml b/config/feature_flags/gitlab_com_derisk/select_ancestors_of_paginated_items.yml
new file mode 100644
index 0000000000000..49f53a32d29fd
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/select_ancestors_of_paginated_items.yml
@@ -0,0 +1,9 @@
+---
+name: select_ancestors_of_paginated_items
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/418749
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108022
+rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/17517
+milestone: '16.9'
+group: group::tenant scale
+type: gitlab_com_derisk
+default_enabled: false
diff --git a/lib/gitlab/multi_collection_paginator.rb b/lib/gitlab/multi_collection_paginator.rb
index e122f0b931727..9e83f6bd5d97f 100644
--- a/lib/gitlab/multi_collection_paginator.rb
+++ b/lib/gitlab/multi_collection_paginator.rb
@@ -22,8 +22,6 @@ def total_count
@total_count ||= first_collection.size + second_collection.size
end
- private
-
def paginated_first_collection(page)
@first_collection_pages ||= Hash.new do |hash, page|
hash[page] = first_collection.page(page).per(per_page)
@@ -50,6 +48,8 @@ def paginated_second_collection(page)
@second_collection_pages[page]
end
+ private
+
def first_collection_page_count
return @first_collection_page_count if defined?(@first_collection_page_count)
diff --git a/spec/finders/group_descendants_finder_spec.rb b/spec/finders/group_descendants_finder_spec.rb
index 14cbb6a427c99..ce768a36ffa18 100644
--- a/spec/finders/group_descendants_finder_spec.rb
+++ b/spec/finders/group_descendants_finder_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe GroupDescendantsFinder do
+RSpec.describe GroupDescendantsFinder, feature_category: :groups_and_projects do
let_it_be(:user) { create(:user) }
let_it_be_with_reload(:group) do
@@ -17,208 +17,193 @@
described_class.new(current_user: user, parent_group: group, params: params)
end
- describe '#has_children?' do
- it 'is true when there are projects' do
- create(:project, namespace: group)
-
- expect(finder.has_children?).to be_truthy
- end
-
- context 'when there are subgroups' do
- it 'is true when there are projects' do
- create(:group, parent: group)
-
- expect(finder.has_children?).to be_truthy
- end
- end
- end
-
- describe '#execute' do
- it 'includes projects' do
- project = create(:project, namespace: group)
-
- expect(finder.execute).to contain_exactly(project)
- end
-
- context 'when archived is `true`' do
- let(:params) { { archived: 'true' } }
-
- it 'includes archived projects' do
- archived_project = create(:project, namespace: group, archived: true)
+ shared_examples 'group descendants finder' do
+ describe '#execute' do
+ it 'includes projects' do
project = create(:project, namespace: group)
- expect(finder.execute).to contain_exactly(archived_project, project)
+ expect(finder.execute).to contain_exactly(project)
end
- end
- context 'when archived is `only`' do
- let(:params) { { archived: 'only' } }
+ context 'when archived is `true`' do
+ let(:params) { { archived: 'true' } }
- it 'includes only archived projects' do
- archived_project = create(:project, namespace: group, archived: true)
- _project = create(:project, namespace: group)
+ it 'includes archived projects' do
+ archived_project = create(:project, namespace: group, archived: true)
+ project = create(:project, namespace: group)
- expect(finder.execute).to contain_exactly(archived_project)
+ expect(finder.execute).to contain_exactly(archived_project, project)
+ end
end
- end
- it 'does not include archived projects' do
- _archived_project = create(:project, :archived, namespace: group)
+ context 'when archived is `only`' do
+ let(:params) { { archived: 'only' } }
- expect(finder.execute).to be_empty
- end
+ it 'includes only archived projects' do
+ archived_project = create(:project, namespace: group, archived: true)
+ _project = create(:project, namespace: group)
- it 'does not include projects aimed for deletion' do
- _project_aimed_for_deletion = create(:project, :archived, marked_for_deletion_at: 2.days.ago, pending_delete: false)
+ expect(finder.execute).to contain_exactly(archived_project)
+ end
+ end
- expect(finder.execute).to be_empty
- end
+ it 'does not include archived projects' do
+ _archived_project = create(:project, :archived, namespace: group)
- context 'with a filter' do
- let(:params) { { filter: 'test' } }
+ expect(finder.execute).to be_empty
+ end
- it 'includes only projects matching the filter' do
- _other_project = create(:project, namespace: group)
- matching_project = create(:project, namespace: group, name: 'testproject')
+ it 'does not include projects aimed for deletion' do
+ _project_aimed_for_deletion =
+ create(:project, :archived, marked_for_deletion_at: 2.days.ago, pending_delete: false)
- expect(finder.execute).to contain_exactly(matching_project)
+ expect(finder.execute).to be_empty
end
- end
- it 'sorts elements by name as default' do
- project1 = create(:project, namespace: group, name: 'z')
- project2 = create(:project, namespace: group, name: 'a')
+ context 'with a filter' do
+ let(:params) { { filter: 'test' } }
- expect(subject.execute).to match_array([project2, project1])
- end
+ it 'includes only projects matching the filter' do
+ _other_project = create(:project, namespace: group)
+ matching_project = create(:project, namespace: group, name: 'testproject')
- context 'sorting by name' do
- let!(:project1) { create(:project, namespace: group, name: 'a', path: 'project-a') }
- let!(:project2) { create(:project, namespace: group, name: 'z', path: 'project-z') }
- let(:params) do
- {
- sort: 'name_asc'
- }
+ expect(finder.execute).to contain_exactly(matching_project)
+ end
end
- it 'sorts elements by name' do
- expect(subject.execute).to eq(
- [
- project1,
- project2
- ]
- )
+ it 'sorts elements by name as default' do
+ project1 = create(:project, namespace: group, name: 'z')
+ project2 = create(:project, namespace: group, name: 'a')
+
+ expect(subject.execute).to match_array([project2, project1])
end
- context 'with nested groups' do
- let!(:subgroup1) { create(:group, parent: group, name: 'a', path: 'sub-a') }
- let!(:subgroup2) { create(:group, parent: group, name: 'z', path: 'sub-z') }
+ context 'sorting by name' do
+ let!(:project1) { create(:project, namespace: group, name: 'a', path: 'project-a') }
+ let!(:project2) { create(:project, namespace: group, name: 'z', path: 'project-z') }
+ let(:params) do
+ {
+ sort: 'name_asc'
+ }
+ end
it 'sorts elements by name' do
expect(subject.execute).to eq(
[
- subgroup1,
- subgroup2,
project1,
project2
]
)
end
- end
- 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: Gitlab::Access::MAINTAINER
- )
- expect(finder.execute).to contain_exactly(project)
- end
- end
+ context 'with nested groups' do
+ let!(:subgroup1) { create(:group, parent: group, name: 'a', path: 'sub-a') }
+ let!(:subgroup2) { create(:group, parent: group, name: 'z', path: 'sub-z') }
+
+ it 'sorts elements by name' do
+ expect(subject.execute).to eq(
+ [
+ subgroup1,
+ subgroup2,
+ project1,
+ project2
+ ]
+ )
+ end
+ end
+ end
- context 'with shared groups' do
- let_it_be(:other_group) { create(:group) }
- let_it_be(:shared_group_link) do
- create(
- :group_group_link,
- shared_group: group,
- shared_with_group: other_group
- )
- 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: Gitlab::Access::MAINTAINER
+ )
- context 'without common ancestor' do
- it { expect(finder.execute).to be_empty }
+ expect(finder.execute).to contain_exactly(project)
+ end
end
- context 'with common ancestor' do
- let_it_be(:common_ancestor) { create(:group) }
- let_it_be(:other_group) { create(:group, parent: common_ancestor) }
- let_it_be(:group) { create(:group, parent: common_ancestor) }
+ context 'with shared groups' do
+ let_it_be(:other_group) { create(:group) }
+ let_it_be(:shared_group_link) do
+ create(
+ :group_group_link,
+ shared_group: group,
+ shared_with_group: other_group
+ )
+ end
- context 'querying under the common ancestor' do
+ context 'without common ancestor' do
it { expect(finder.execute).to be_empty }
end
- context 'querying the common ancestor' do
- subject(:finder) do
- described_class.new(current_user: user, parent_group: common_ancestor, params: params)
+ context 'with common ancestor' do
+ let_it_be(:common_ancestor) { create(:group) }
+ let_it_be(:other_group) { create(:group, parent: common_ancestor) }
+ let_it_be(:group) { create(:group, parent: common_ancestor) }
+
+ context 'querying under the common ancestor' do
+ it { expect(finder.execute).to be_empty }
end
- it 'contains shared subgroups' do
- expect(finder.execute).to contain_exactly(group, other_group)
+ context 'querying the common ancestor' do
+ subject(:finder) do
+ described_class.new(current_user: user, parent_group: common_ancestor, params: params)
+ end
+
+ it 'contains shared subgroups' do
+ expect(finder.execute).to contain_exactly(group, other_group)
+ end
end
end
end
- end
- context 'with nested groups' do
- let_it_be(:project) { create(:project, namespace: group) }
- let_it_be_with_reload(:subgroup) { create(:group, :private, parent: group) }
+ context 'with nested groups' do
+ let_it_be(:project) { create(:project, namespace: group) }
+ let_it_be_with_reload(:subgroup) { create(:group, :private, parent: group) }
- describe '#execute' do
- it 'contains projects and subgroups' do
- expect(finder.execute).to contain_exactly(subgroup, project)
- end
+ describe '#execute' do
+ it 'contains projects and subgroups' do
+ expect(finder.execute).to contain_exactly(subgroup, project)
+ end
- it 'does not include subgroups the user does not have access to' do
- subgroup.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+ it 'does not include subgroups the user does not have access to' do
+ subgroup.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
- public_subgroup = create(:group, :public, parent: group, path: 'public-group')
- other_subgroup = create(:group, :private, parent: group, path: 'visible-private-group')
- other_user = create(:user)
- other_subgroup.add_developer(other_user)
+ public_subgroup = create(:group, :public, parent: group, path: 'public-group')
+ other_subgroup = create(:group, :private, parent: group, path: 'visible-private-group')
+ other_user = create(:user)
+ other_subgroup.add_developer(other_user)
- finder = described_class.new(current_user: other_user, parent_group: group)
+ finder = described_class.new(current_user: other_user, parent_group: group)
- expect(finder.execute).to contain_exactly(public_subgroup, other_subgroup)
- end
+ expect(finder.execute).to contain_exactly(public_subgroup, other_subgroup)
+ end
- it 'only includes public groups when no user is given' do
- public_subgroup = create(:group, :public, parent: group)
- _private_subgroup = create(:group, :private, parent: group)
+ it 'only includes public groups when no user is given' do
+ public_subgroup = create(:group, :public, parent: group)
+ _private_subgroup = create(:group, :private, parent: group)
- finder = described_class.new(current_user: nil, parent_group: group)
+ finder = described_class.new(current_user: nil, parent_group: group)
- expect(finder.execute).to contain_exactly(public_subgroup)
- end
+ expect(finder.execute).to contain_exactly(public_subgroup)
+ end
- context 'when archived is `true`' do
- let(:params) { { archived: 'true' } }
+ context 'when archived is `true`' do
+ let(:params) { { archived: 'true' } }
- it 'includes archived projects in the count of subgroups' do
- create(:project, namespace: subgroup, archived: true)
+ it 'includes archived projects in the count of subgroups' do
+ create(:project, namespace: subgroup, archived: true)
- expect(finder.execute.first.preloaded_project_count).to eq(1)
+ expect(finder.execute.first.preloaded_project_count).to eq(1)
+ end
end
- end
- context 'with a filter' do
- let(:params) { { filter: 'test' } }
+ context 'with a filter' do
+ let(:params) { { filter: 'test' } }
- shared_examples 'filter examples' do
it 'contains only matching projects and subgroups' do
matching_project = create(:project, namespace: group, name: 'Testproject')
matching_subgroup = create(:group, name: 'testgroup', parent: group)
@@ -274,8 +259,36 @@
end
end
end
+ end
+ end
+ end
+
+ it_behaves_like 'group descendants finder'
+
+ context 'with page param' do
+ let_it_be(:params) { { page: 2, per_page: 1, filter: 'test' } }
+ let_it_be(:matching_subgroup1) { create(:group, :private, name: 'testgroup1', parent: group) }
+ let_it_be(:matching_subgroup2) { create(:group, :private, name: 'testgroup2', parent: group) }
+
+ it 'does not include items from previous pages' do
+ expect(finder.execute).to contain_exactly(matching_subgroup2)
+ end
+ end
+
+ context 'with select_ancestors_of_paginated_items feature flag disabled' do
+ before do
+ stub_feature_flags(select_ancestors_of_paginated_items: false)
+ end
+
+ it_behaves_like 'group descendants finder'
+
+ context 'with page param' do
+ let_it_be(:params) { { page: 2, per_page: 1, filter: 'test' } }
+ let_it_be(:matching_subgroup1) { create(:group, :private, name: 'testgroup1', parent: group) }
+ let_it_be(:matching_subgroup2) { create(:group, :private, name: 'testgroup2', parent: group) }
- it_behaves_like 'filter examples'
+ it 'does includes items from previous pages' do
+ expect(finder.execute).to contain_exactly(matching_subgroup1, matching_subgroup2)
end
end
end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 57482a604e27e..ce2c9ced47f03 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -1476,6 +1476,13 @@
expect(group.member_owners_excluding_project_bots).to contain_exactly(member_owner)
end
+ it 'preloads user and source' do
+ owner = group.member_owners_excluding_project_bots.first
+
+ expect(owner.association(:user).loaded?).to be_truthy
+ expect(owner.association(:source).loaded?).to be_truthy
+ end
+
context 'there is also a project_bot owner' do
before do
group.add_member(create(:user, :project_bot), GroupMember::OWNER)
diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml
index 2d69d6612b45e..c6be82ec059f4 100644
--- a/spec/support/rspec_order_todo.yml
+++ b/spec/support/rspec_order_todo.yml
@@ -4120,7 +4120,6 @@
- './spec/finders/feature_flags_user_lists_finder_spec.rb'
- './spec/finders/fork_projects_finder_spec.rb'
- './spec/finders/fork_targets_finder_spec.rb'
-- './spec/finders/group_descendants_finder_spec.rb'
- './spec/finders/group_members_finder_spec.rb'
- './spec/finders/group_projects_finder_spec.rb'
- './spec/finders/groups/accepting_project_transfers_finder_spec.rb'
--
GitLab