From cd15b91b15598ee72dd4972a602306dd09152ad5 Mon Sep 17 00:00:00 2001
From: Adam Hegyi <ahegyi@gitlab.com>
Date: Tue, 7 Feb 2023 12:58:38 +0100
Subject: [PATCH] Preload max access level when loading group labels
Preload max access level when loading group labels to improve
LabelsFinder performance. The change is behind a feature flag:
preload_max_access_levels_for_labels_finder
---
.../concerns/finder_with_group_hierarchy.rb | 6 ++
...ad_max_access_levels_for_labels_finder.yml | 8 ++
.../finder_with_group_hierarchy_spec.rb | 98 ++++++++++++++++++-
3 files changed, 111 insertions(+), 1 deletion(-)
create mode 100644 config/feature_flags/development/preload_max_access_levels_for_labels_finder.yml
diff --git a/app/finders/concerns/finder_with_group_hierarchy.rb b/app/finders/concerns/finder_with_group_hierarchy.rb
index 86ccac19b63a0..4ced544ba2cf4 100644
--- a/app/finders/concerns/finder_with_group_hierarchy.rb
+++ b/app/finders/concerns/finder_with_group_hierarchy.rb
@@ -27,6 +27,12 @@ def group_ids_for(group)
# we can preset root group for all of them to optimize permission checks
Group.preset_root_ancestor_for(groups)
+ # Preloading the max access level for the given groups to avoid N+1 queries
+ # during the access check.
+ if !skip_authorization && current_user && Feature.enabled?(:preload_max_access_levels_for_labels_finder, group)
+ Preloaders::UserMaxAccessLevelInGroupsPreloader.new(groups, current_user).execute
+ end
+
groups_user_can_read_items(groups).map(&:id)
end
end
diff --git a/config/feature_flags/development/preload_max_access_levels_for_labels_finder.yml b/config/feature_flags/development/preload_max_access_levels_for_labels_finder.yml
new file mode 100644
index 0000000000000..456616384327f
--- /dev/null
+++ b/config/feature_flags/development/preload_max_access_levels_for_labels_finder.yml
@@ -0,0 +1,8 @@
+---
+name: preload_max_access_levels_for_labels_finder
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111313
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/390664
+milestone: '15.9'
+type: development
+group: group::optimize
+default_enabled: false
diff --git a/spec/finders/concerns/finder_with_group_hierarchy_spec.rb b/spec/finders/concerns/finder_with_group_hierarchy_spec.rb
index 8c2026a00a11a..c96e35372d6f8 100644
--- a/spec/finders/concerns/finder_with_group_hierarchy_spec.rb
+++ b/spec/finders/concerns/finder_with_group_hierarchy_spec.rb
@@ -40,7 +40,7 @@ def read_permission
let_it_be(:private_group) { create(:group, :private) }
let_it_be(:private_subgroup) { create(:group, :private, parent: private_group) }
- let(:user) { create(:user) }
+ let!(:user) { create(:user) }
context 'when specifying group' do
it 'returns only the group by default' do
@@ -109,4 +109,100 @@ def read_permission
expect(finder.execute(skip_authorization: true)).to match_array([private_group.id, private_subgroup.id])
end
end
+
+ context 'with N+1 query check' do
+ def run_query(group)
+ finder_class
+ .new(user, group: group, include_descendant_groups: true)
+ .execute
+ .to_a
+
+ RequestStore.clear!
+ end
+
+ it 'does not produce N+1 query', :request_store do
+ private_group.add_developer(user)
+
+ run_query(private_subgroup) # warmup
+ control = ActiveRecord::QueryRecorder.new { run_query(private_subgroup) }
+
+ expect { run_query(private_group) }.not_to exceed_query_limit(control)
+ end
+ end
+
+ context 'when preload_max_access_levels_for_labels_finder is disabled' do
+ # All test cases were copied from above, these will be removed once the FF is removed.
+
+ before do
+ stub_feature_flags(preload_max_access_levels_for_labels_finder: false)
+ end
+
+ context 'when specifying group' do
+ it 'returns only the group by default' do
+ finder = finder_class.new(user, group: group)
+
+ expect(finder.execute).to match_array([group.id])
+ end
+ end
+
+ context 'when specifying group_id' do
+ it 'returns only the group by default' do
+ finder = finder_class.new(user, group_id: group.id)
+
+ expect(finder.execute).to match_array([group.id])
+ end
+ end
+
+ context 'when including items from group ancestors' do
+ before do
+ private_subgroup.add_developer(user)
+ end
+
+ it 'returns group and its ancestors' do
+ private_group.add_developer(user)
+
+ finder = finder_class.new(user, group: private_subgroup, include_ancestor_groups: true)
+
+ expect(finder.execute).to match_array([private_group.id, private_subgroup.id])
+ end
+
+ it 'ignores groups which user can not read' do
+ finder = finder_class.new(user, group: private_subgroup, include_ancestor_groups: true)
+
+ expect(finder.execute).to match_array([private_subgroup.id])
+ end
+
+ it 'returns them all when skip_authorization is true' do
+ finder = finder_class.new(user, group: private_subgroup, include_ancestor_groups: true)
+
+ expect(finder.execute(skip_authorization: true)).to match_array([private_group.id, private_subgroup.id])
+ end
+ end
+
+ context 'when including items from group descendants' do
+ before do
+ private_subgroup.add_developer(user)
+ end
+
+ it 'returns items from group and its descendants' do
+ private_group.add_developer(user)
+
+ finder = finder_class.new(user, group: private_group, include_descendant_groups: true)
+
+ expect(finder.execute).to match_array([private_group.id, private_subgroup.id])
+ end
+
+ it 'ignores items from groups which user can not read' do
+ finder = finder_class.new(user, group: private_group, include_descendant_groups: true)
+
+ expect(finder.execute).to match_array([private_subgroup.id])
+ end
+
+ it 'returns them all when skip_authorization is true' do
+ finder = finder_class.new(user, group: private_group, include_descendant_groups: true)
+
+ expect(finder.execute(skip_authorization: true)).to match_array([private_group.id, private_subgroup.id])
+ end
+ end
+ end
end
--
GitLab