Merge branch 'group_epics_count' into 'master'

Optimize subgroup permission checking

See merge request gitlab-org/gitlab!35061

ee/app/finders/epics_finder.rb

@@ -75,9 +75,7 @@ class EpicsFinder < IssuableFinder
                # The `group` method takes care of checking permissions
                [group]
              else
-               # `same_root` should be set only if we are sure that all groups
-               # in related_groups have the same ancestor root group
-               ::Group.groups_user_can_read_epics(related_groups, current_user, same_root: true)
+               permissioned_related_groups
              end
 
     epics = Epic.where(group: groups)
@@ -87,6 +85,22 @@ class EpicsFinder < IssuableFinder
 
   private
 
+  def permissioned_related_groups
+    groups = related_groups
+
+    # if user is member of top-level related group, he can automatically read
+    # all epics in all subgroups
+    return groups if can_read_all_epics_in_related_groups?(groups)
+
+    groups_user_can_read_epics(groups)
+  end
+
+  def groups_user_can_read_epics(groups)
+    # `same_root` should be set only if we are sure that all groups
+    # in related_groups have the same ancestor root group
+    ::Group.groups_user_can_read_epics(groups, current_user, same_root: true)
+  end
+
   def filter_items(items)
     items = by_created_at(items)
     items = by_updated_at(items)
@@ -166,7 +180,7 @@ class EpicsFinder < IssuableFinder
 
   def with_confidentiality_access_check(epics, groups)
     return epics unless Feature.enabled?(:confidential_epics_query, group)
-    return epics if can_read_all_related_groups?(groups)
+    return epics if can_read_all_epics_in_related_groups?(groups)
 
     epics.not_confidential_or_in_groups(groups_with_confidential_access(groups))
   end
@@ -179,7 +193,7 @@ class EpicsFinder < IssuableFinder
     GroupMember.by_group_ids(group_ids).by_user_id(current_user).non_guests.select(:source_id)
   end
 
-  def can_read_all_related_groups?(groups)
+  def can_read_all_epics_in_related_groups?(groups)
     return false unless current_user
 
     # If a user is a member of a group, he also inherits access to all subgroups,

ee/changelogs/unreleased/group_epics_count.yml

+---
+title: Optimize permission checking when finding subgroup epics.
+merge_request: 35061
+author:
+type: performance

ee/spec/finders/epics_finder_spec.rb

@@ -57,8 +57,8 @@ RSpec.describe EpicsFinder do
           expect(epics).to contain_exactly(epic1, epic2, epic3)
         end
 
-        it 'does not execute more than 8 SQL queries' do
-          expect { epics.to_a }.not_to exceed_all_query_limit(8)
+        it 'does not execute more than 5 SQL queries' do
+          expect { epics.to_a }.not_to exceed_all_query_limit(5)
         end
 
         context 'sorting' do
@@ -169,18 +169,18 @@ RSpec.describe EpicsFinder do
             end
           end
 
-          it 'does not execute more than 12 SQL queries' do
-            expect { epics.to_a }.not_to exceed_all_query_limit(12)
+          it 'does not execute more than 5 SQL queries' do
+            expect { epics.to_a }.not_to exceed_all_query_limit(5)
           end
 
-          it 'does not execute more than 13 SQL queries when checking namespace plans' do
+          it 'does not execute more than 6 SQL queries when checking namespace plans' do
             allow(Gitlab::CurrentSettings)
               .to receive(:should_check_namespace_plan?)
               .and_return(true)
 
             create(:gitlab_subscription, :gold, namespace: group)
 
-            expect { epics.to_a }.not_to exceed_all_query_limit(13)
+            expect { epics.to_a }.not_to exceed_all_query_limit(6)
           end
         end
 
@@ -323,6 +323,18 @@ RSpec.describe EpicsFinder do
             it 'returns all nested epics' do
               expect(subject).to match_array([base_epic1, base_epic2, private_epic1, private_epic2, public_epic1, public_epic2])
             end
+
+            it 'does not execute more than 5 SQL queries' do
+              expect { subject }.not_to exceed_all_query_limit(5)
+            end
+
+            it 'does not check permission for subgroups because user inherits permission' do
+              finder = described_class.new(search_user, group_id: base_group.id)
+
+              expect(finder).not_to receive(:groups_user_can_read_epics)
+
+              finder.execute
+            end
           end
 
           context 'when user is member of private subgroup' do
@@ -333,6 +345,20 @@ RSpec.describe EpicsFinder do
             it 'returns also confidential epics from this subgroup' do
               expect(subject).to match_array([base_epic2, private_epic1, private_epic2, public_epic1])
             end
+
+            # if user is not member of top-level group, we need to check
+            # if he can read epics in each subgroup
+            it 'does not execute more than 10 SQL queries' do
+              expect { subject }.not_to exceed_all_query_limit(10)
+            end
+
+            it 'checks permission for each subgroup' do
+              finder = described_class.new(search_user, group_id: base_group.id)
+
+              expect(finder).to receive(:groups_user_can_read_epics).and_call_original
+
+              finder.execute
+            end
           end
 
           context 'when user is member of public subgroup' do