Merge branch...

Merge branch '21033-controller-groups-groupmemberscontroller-index-executes-more-than-100-sql-queries-p80-108-6' into 'master'

Resolve created_by and oncall_schedule n+1's in group member index

See merge request gitlab-org/gitlab!59756

app/models/members_preloader.rb

@@ -10,9 +10,10 @@ class MembersPreloader
   def preload_all
     ActiveRecord::Associations::Preloader.new.preload(members, :user)
     ActiveRecord::Associations::Preloader.new.preload(members, :source)
-    ActiveRecord::Associations::Preloader.new.preload(members.map(&:user), :status)
-    ActiveRecord::Associations::Preloader.new.preload(members.map(&:user), :u2f_registrations)
-    ActiveRecord::Associations::Preloader.new.preload(members.map(&:user), :webauthn_registrations)
+    ActiveRecord::Associations::Preloader.new.preload(members, :created_by)
+    ActiveRecord::Associations::Preloader.new.preload(members, user: :status)
+    ActiveRecord::Associations::Preloader.new.preload(members, user: :u2f_registrations)
+    ActiveRecord::Associations::Preloader.new.preload(members, user: :webauthn_registrations)
   end
 end
 

ee/app/models/ee/members_preloader.rb

@@ -8,9 +8,9 @@ module EE
     def preload_all
       super
 
-      users = members.map(&:user)
-      ActiveRecord::Associations::Preloader.new.preload(users, group_saml_identities: :saml_provider)
-      ActiveRecord::Associations::Preloader.new.preload(users, oncall_participants: { rotation: :schedule })
+      ActiveRecord::Associations::Preloader.new.preload(members, user: { group_saml_identities: :saml_provider })
+      ActiveRecord::Associations::Preloader.new.preload(members, user: { oncall_participants: { rotation: :schedule } })
+      ActiveRecord::Associations::Preloader.new.preload(members, user: :oncall_schedules)
     end
   end
 end

ee/changelogs/unreleased/21033-controller-groups-groupmemberscontroller-index-executes-more-tha.yml

+---
+title: Resolve created_by and oncall_schedule n+1's in group member index
+merge_request: 59756
+author:
+type: performance

ee/spec/controllers/groups/group_members_controller_spec.rb

@@ -14,6 +14,33 @@ RSpec.describe Groups::GroupMembersController do
     sign_in(user)
   end
 
+  describe 'GET #index' do
+    context 'with members, invites and requests queries' do
+      render_views
+
+      let!(:invited) { create(:group_member, :invited, :developer, group: group) }
+      let!(:requested) { create(:group_member, :access_request, group: group) }
+
+      it 'records queries', :use_sql_query_cache do
+        get :index, params: { group_id: group }
+
+        control = ActiveRecord::QueryRecorder.new(skip_cached: false) { get :index, params: { group_id: group } }
+        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 = 36
+        # created_by = 8 # 88 vs 134
+        # oncall_schedules = 6 # 87 vs 128
+        multiple_members_threshold = 5
+
+        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
+    end
+  end
+
   describe 'POST #create' do
     it 'creates an audit event' do
       expect do

spec/serializers/member_serializer_spec.rb

@@ -30,39 +30,6 @@ RSpec.describe MemberSerializer do
                                      .from(nil).to(true)
                                      .and change(group_member, :last_blocked_owner).from(nil).to(false)
     end
-
-    context "with LastGroupOwnerAssigner query improvements" do
-      it "avoids N+1 database queries for last group owner assignment in MembersPresenter" do
-        group_member = create(:group_member, group: group)
-        control_count = ActiveRecord::QueryRecorder.new { member_last_owner_with_preload([group_member]) }.count
-        group_members = create_list(:group_member, 3, group: group)
-
-        expect { member_last_owner_with_preload(group_members) }.not_to exceed_query_limit(control_count)
-      end
-
-      it "avoids N+1 database queries for last blocked owner assignment in MembersPresenter" do
-        group_member = create(:group_member, group: group)
-        control_count = ActiveRecord::QueryRecorder.new { member_last_blocked_owner_with_preload([group_member]) }.count
-        group_members = create_list(:group_member, 3, group: group)
-
-        expect { member_last_blocked_owner_with_preload(group_members) }.not_to exceed_query_limit(control_count)
-      end
-
-      def member_last_owner_with_preload(members)
-        assigner_with_preload(members)
-        members.map { |m| group.member_last_owner?(m) }
-      end
-
-      def member_last_blocked_owner_with_preload(members)
-        assigner_with_preload(members)
-        members.map { |m| group.member_last_blocked_owner?(m) }
-      end
-
-      def assigner_with_preload(members)
-        MembersPreloader.new(members).preload_all
-        Members::LastGroupOwnerAssigner.new(group, members).execute
-      end
-    end
   end
 
   context 'project member' do