Merge branch '329778-fine-tune-a-few-queries-found-in-groupmembers-index' into 'master'

Fine tune a few queries found in GroupMembers#index

See merge request gitlab-org/gitlab!60857

app/controllers/groups/group_members_controller.rb

@@ -4,6 +4,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
   include MembershipActions
   include MembersPresentation
   include SortingHelper
+  include Gitlab::Utils::StrongMemoize
 
   MEMBER_PER_PAGE_LIMIT = 50
 
@@ -21,6 +22,8 @@ class Groups::GroupMembersController < Groups::ApplicationController
 
   feature_category :authentication_and_authorization
 
+  helper_method :can_manage_members?
+
   def index
     preload_max_access
     @sort = params[:sort].presence || sort_value_name
@@ -29,7 +32,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
       .new(@group, current_user, params: filter_params)
       .execute(include_relations: requested_relations)
 
-    if can_manage_members
+    if can_manage_members?
       @skip_groups = @group.related_group_ids
 
       @invited_members = @members.invite
@@ -59,8 +62,10 @@ class Groups::GroupMembersController < Groups::ApplicationController
     current_user.max_access_for_group[@group.id] = @group.max_member_access(current_user)
   end
 
-  def can_manage_members
-    can?(current_user, :admin_group_member, @group)
+  def can_manage_members?
+    strong_memoize(:can_manage_members) do
+      can?(current_user, :admin_group_member, @group)
+    end
   end
 
   def present_invited_members(invited_members)

app/views/groups/group_members/index.html.haml

 - add_page_specific_style 'page_bundles/members'
 - page_title _('Group members')
-- can_manage_members = can?(current_user, :admin_group_member, @group)
-- show_invited_members = can_manage_members && @invited_members.exists?
-- show_access_requests = can_manage_members && @requesters.exists?
+- show_invited_members = can_manage_members? && @invited_members.load.any?
+- show_access_requests = can_manage_members? && @requesters.load.any?
 - invited_active = params[:search_invited].present? || params[:invited_members_page].present?
 
 .js-remove-member-modal
 .row.gl-mt-3
   .col-lg-12
     .gl-display-flex.gl-flex-wrap
-      - if can_manage_members
+      - if can_manage_members?
         .gl-w-half.gl-xs-w-full
           %h4
             = _('Group members')
@@ -21,7 +20,7 @@
               .js-invite-group-trigger{ data: { classes: 'gl-mt-3 gl-sm-w-auto gl-w-full', display_text: _('Invite a group') } }
               .js-invite-members-trigger{ data: { variant: 'success', classes: 'gl-mt-3 gl-sm-w-auto gl-w-full gl-sm-ml-3', display_text: _('Invite members') } }
           = render 'groups/invite_members_modal', group: @group
-    - if can_manage_members && Feature.disabled?(:invite_members_group_modal, @group)
+    - if can_manage_members? && Feature.disabled?(:invite_members_group_modal, @group)
       %hr.gl-mt-4
         %ul.nav-links.nav.nav-tabs.gitlab-tabs{ role: 'tablist' }
           %li.nav-tab{ role: 'presentation' }
@@ -42,7 +41,7 @@
           %span
             = _('Members')
           %span.badge.gl-tab-counter-badge.badge-muted.badge-pill.gl-badge.sm= @members.total_count
-      - if @group.shared_with_group_links.any?
+      - if @group.shared_with_group_links.present?
         %li.nav-item
           = link_to '#tab-groups', class: ['nav-link'] , data: { toggle: 'tab', qa_selector: 'groups_list_tab' } do
             %span
@@ -65,7 +64,7 @@
         .js-group-members-list{ data: group_members_list_data_attributes(@group, @members, { param_name: :page, params: { invited_members_page: nil, search_invited: nil } }) }
           .loading
             .gl-spinner.gl-spinner-md
-      - if @group.shared_with_group_links.any?
+      - if @group.shared_with_group_links.present?
         #tab-groups.tab-pane
           .js-group-group-links-list{ data: group_group_links_list_data_attributes(@group) }
             .loading

changelogs/unreleased/329778-fine-tune-a-few-queries-found-in-groupmembers-index.yml

+---
+title: Fine tune a few queries found in GroupMembers#index
+merge_request: 60857
+author:
+type: performance

ee/spec/controllers/groups/group_members_controller_spec.rb

@@ -27,20 +27,21 @@ RSpec.describe Groups::GroupMembersController do
         create_list(:group_member, 5, group: group, created_by: user)
         create_list(:group_member, 5, :invited, :developer, group: group, created_by: user)
         create_list(:group_member, 5, :access_request, group: group)
-        # locally 87 vs 128
-        unresolved_n_plus_ones = 8 # 1 in GDK, 5 in CI hard to say - multiple should likely be lower now
-        # using_license 84 vs 115 = ~13
-        # can_update 82 vs 102 = ~13
-        # can_remove 80 vs 89 = ~13
-        # can_resend 80 + 5 = ~4
-        # solving access level reduced from ~80 to ~50
-        # still have a few queries created by can_update/can_remove that should be reduced
-        multiple_members_threshold = 5
+        # locally 47 vs 52 GDK vs 57 CI
+        unresolved_n_plus_ones = 5 # still have a few queries created by can_update/can_remove that should be reduced
+        multiple_members_threshold = 5 # GDK vs CI difference
 
         expect do
           get :index, params: { group_id: group.reload }
         end.not_to exceed_all_query_limit(control.count).with_threshold(multiple_members_threshold + unresolved_n_plus_ones)
       end
+
+      it 'avoids extra group_link database queries utilizing pre-loading' do
+        control = ActiveRecord::QueryRecorder.new { get :index, params: { group_id: group } }
+        count_queries = control.occurrences_by_line_method.first[1][:occurrences].any? { |i| i.include?('SELECT 1 AS one FROM "group_group_links" WHERE "group_group_links"') }
+
+        expect(count_queries).to be(false)
+      end
     end
   end
 

spec/controllers/groups/group_members_controller_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe Groups::GroupMembersController do
       expect(response).to render_template(:index)
     end
 
-    context 'user with owner access' do
+    context 'when user can manage members' do
       let_it_be(:invited) { create_list(:group_member, 3, :invited, group: group) }
 
       before do
@@ -71,6 +71,19 @@ RSpec.describe Groups::GroupMembersController do
       end
     end
 
+    context 'when user cannot manage members' do
+      before do
+        sign_in(user)
+      end
+
+      it 'does not assign invited members or skip_groups', :aggregate_failures do
+        get :index, params: { group_id: group }
+
+        expect(assigns(:invited_members)).to be_nil
+        expect(assigns(:skip_groups)).to be_nil
+      end
+    end
+
     context 'when user has owner access to subgroup' do
       let_it_be(:nested_group) { create(:group, parent: group) }
       let_it_be(:nested_group_user) { create(:user) }