Merge branch '327106-fix-epics-search-count-n-plus-1-queries' into 'master'

Fix N+1 query for EpicsFinder when guest is searching in a private group

See merge request gitlab-org/gitlab!58863

ee/app/finders/epics_finder.rb

@@ -95,7 +95,7 @@ class EpicsFinder < IssuableFinder
 
       # if user is member of top-level related group, he can automatically read
       # all epics in all subgroups
-      next groups if can_read_all_epics_in_related_groups?(groups)
+      next groups if can_read_all_epics_in_related_groups?(groups, include_confidential: false)
 
       groups_user_can_read_epics(groups)
     end
@@ -204,7 +204,14 @@ class EpicsFinder < IssuableFinder
     GroupMember.by_group_ids(group_ids).by_user_id(current_user).non_guests.select(:source_id)
   end
 
-  def can_read_all_epics_in_related_groups?(groups)
+  # @param include_confidential [Boolean] if this method should factor in
+  # confidential issues. Setting this to `false` will mean that it only checks
+  # the user can view all non-confidential epics within all of these groups. It
+  # does not check that they can view confidential epics and as such may return
+  # `true` even if `groups` contains a group where the user cannot view
+  # confidential epics. As such you should only call this with `false` if you
+  # are planning on filtering out confidential epics separately.
+  def can_read_all_epics_in_related_groups?(groups, include_confidential: true)
     return true if @skip_visibility_check
     return false unless current_user
 
@@ -219,7 +226,19 @@ class EpicsFinder < IssuableFinder
     # descending order (so top-level first), except if we fetch ancestors
     # - in that case top-level group is group's root parent
     parent = params.fetch(:include_ancestor_groups, false) ? groups.first.root_ancestor : group
-    Ability.allowed?(current_user, :read_confidential_epic, parent)
+
+    # If they can view confidential epics in this parent group they can
+    # definitely view confidential epics in subgroups.
+    return true if Ability.allowed?(current_user, :read_confidential_epic, parent)
+
+    # If we don't account for confidential (assume it will be filtered later by
+    # with_confidentiality_access_check) then as long as the user can see all
+    # epics in this group they can see in all subgroups. This is only true for
+    # private top level groups because it's possible that a top level public
+    # group has private subgroups and therefore they would not necessarily be
+    # able to read epics in the private subgroup even though they can in the
+    # parent group.
+    !include_confidential && parent.private? && Ability.allowed?(current_user, :read_epic, parent)
   end
 
   def by_confidential(items)

ee/changelogs/unreleased/327106-fix-epics-search-count-n-plus-1-queries.yml

+---
+title: Fix N+1 query in Epics search for guest in private group
+merge_request: 58863
+author:
+type: performance

ee/spec/finders/epics_finder_spec.rb

@@ -184,6 +184,19 @@ RSpec.describe EpicsFinder do
                 it { is_expected.to contain_exactly(subgroup_epic, subgroup2_epic, epic1, epic2, epic3) }
               end
             end
+
+            context 'when user is a guest of top level group' do
+              it 'does not have N+1 queries for subgroups' do
+                GroupMember.where(user_id: search_user.id).delete_all
+                group.add_guest(search_user)
+
+                control = ActiveRecord::QueryRecorder.new(skip_cached: false) { epics.to_a }
+
+                create_list(:group, 5, :private, parent: group)
+
+                expect { epics.to_a }.not_to exceed_all_query_limit(control)
+              end
+            end
           end
 
           it 'does not execute more than 5 SQL queries' do
@@ -590,6 +603,16 @@ RSpec.describe EpicsFinder do
             end
           end
 
+          context 'when user is a guest in the base group' do
+            before do
+              base_group.add_guest(search_user)
+            end
+
+            it 'does not return any confidential epics in the base or subgroups' do
+              expect(subject).to match_array([base_epic2, private_epic1, public_epic1])
+            end
+          end
+
           context 'when user is member of public subgroup' do
             before do
               public_group1.add_developer(search_user)