diff --git a/app/finders/concerns/finder_with_group_hierarchy.rb b/app/finders/concerns/finder_with_group_hierarchy.rb index 86ccac19b63a064ee4877ab7e90e24c0a5a78f37..4ced544ba2cf44fcbe323a69e67d9737675a6f3b 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 0000000000000000000000000000000000000000..456616384327f393b66e8f2600c0c8eca803ac4d --- /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 8c2026a00a11aebc560aeae76d9fbb880b7547c0..c96e35372d6f842177742d6d01bab626705e6646 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