Merge branch...

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

Resolve admin_group_member group policy n+1

See merge request gitlab-org/gitlab!58948

app/controllers/groups/group_members_controller.rb

@@ -22,6 +22,7 @@ class Groups::GroupMembersController < Groups::ApplicationController
   feature_category :authentication_and_authorization
 
   def index
+    preload_max_access
     @sort = params[:sort].presence || sort_value_name
 
     @members = GroupMembersFinder
@@ -50,6 +51,14 @@ class Groups::GroupMembersController < Groups::ApplicationController
 
   private
 
+  def preload_max_access
+    return unless current_user
+
+    # this allows the can? against admin type queries in this action to
+    # only perform the query once, even if it is cached
+    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)
   end

app/models/group.rb

@@ -551,11 +551,17 @@ class Group < Namespace
   def max_member_access_for_user(user, only_concrete_membership: false)
     return GroupMember::NO_ACCESS unless user
     return GroupMember::OWNER if user.can_admin_all_resources? && !only_concrete_membership
+    return user.max_access_for_group[id] if user.max_access_for_group[id]
 
-    max_member_access = members_with_parents.where(user_id: user)
-                                            .reorder(access_level: :desc)
-                                            .first
-                                            &.access_level
+    max_member_access(user)
+  end
+
+  def max_member_access(user)
+    max_member_access = members_with_parents
+                          .where(user_id: user)
+                          .reorder(access_level: :desc)
+                          .first
+                          &.access_level
 
     max_member_access || GroupMember::NO_ACCESS
   end

app/models/user.rb

@@ -96,6 +96,12 @@ class User < ApplicationRecord
   # Virtual attribute for impersonator
   attr_accessor :impersonator
 
+  attr_writer :max_access_for_group
+
+  def max_access_for_group
+    @max_access_for_group ||= {}
+  end
+
   #
   # Relations
   #

changelogs/unreleased/21033-controller-groups-groupmemberscontroller-index-executes-more-than-1.yml

+---
+title: Resolve admin_group_member group policy n+1
+merge_request: 58948
+author:
+type: performance

ee/spec/controllers/groups/group_members_controller_spec.rb

@@ -5,9 +5,8 @@ require 'spec_helper'
 RSpec.describe Groups::GroupMembersController do
   include ExternalAuthorizationServiceHelpers
 
-  let(:user)  { create(:user) }
-  let(:group) { create(:group, :public) }
-  let(:membership) { create(:group_member, group: group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group, reload: true) { create(:group, :public) }
 
   before do
     group.add_owner(user)
@@ -29,9 +28,13 @@ RSpec.describe Groups::GroupMembersController do
         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
+        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
 
         expect do
@@ -85,7 +88,8 @@ RSpec.describe Groups::GroupMembersController do
     end
 
     describe 'POST #override' do
-      let(:group) { create(:group_with_ldap_group_link) }
+      let_it_be(:group) { create(:group_with_ldap_group_link) }
+      let_it_be(:membership) { create(:group_member, group: group) }
 
       before do
         allow(Ability).to receive(:allowed?).and_call_original
@@ -105,7 +109,7 @@ RSpec.describe Groups::GroupMembersController do
       end
 
       context 'when user has minimal access' do
-        let(:membership) { create(:group_member, :minimal_access, source: group, user: create(:user)) }
+        let_it_be(:membership) { create(:group_member, :minimal_access, source: group, user: create(:user)) }
 
         it 'is not successful' do
           post :override,
@@ -195,8 +199,8 @@ RSpec.describe Groups::GroupMembersController do
       end
 
       context 'when group has email domain feature disabled' do
-        let(:email) { 'unverified@gitlab.com' }
-        let(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
+        let_it_be(:email) { 'unverified@gitlab.com' }
+        let_it_be(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
 
         before do
           stub_licensed_features(group_allowed_email_domains: false)
@@ -248,8 +252,8 @@ RSpec.describe Groups::GroupMembersController do
       end
 
       context 'when group has email domain feature disabled' do
-        let(:email) { 'unverified@gitlab.com' }
-        let(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
+        let_it_be(:email) { 'unverified@gitlab.com' }
+        let_it_be(:requesting_user) { create(:user, email: email, confirmed_at: nil) }
 
         before do
           stub_licensed_features(group_allowed_email_domains: false)
@@ -272,7 +276,7 @@ RSpec.describe Groups::GroupMembersController do
 
   describe 'POST #resend_invite' do
     context 'when user has minimal access' do
-      let(:membership) { create(:group_member, :minimal_access, source: group, user: create(:user)) }
+      let_it_be(:membership) { create(:group_member, :minimal_access, source: group, user: create(:user)) }
 
       it 'is not successful' do
         post :resend_invite, params: { group_id: group, id: membership }

spec/controllers/groups/group_members_controller_spec.rb

@@ -5,9 +5,8 @@ require 'spec_helper'
 RSpec.describe Groups::GroupMembersController do
   include ExternalAuthorizationServiceHelpers
 
-  let(:user) { create(:user) }
-  let(:group) { create(:group, :public) }
-  let(:membership) { create(:group_member, group: group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group, reload: true) { create(:group, :public) }
 
   before do
     travel_to DateTime.new(2019, 4, 1)
@@ -26,13 +25,21 @@ RSpec.describe Groups::GroupMembersController do
     end
 
     context 'user with owner access' do
-      let!(:invited) { create_list(:group_member, 3, :invited, group: group) }
+      let_it_be(:invited) { create_list(:group_member, 3, :invited, group: group) }
 
       before do
         group.add_owner(user)
         sign_in(user)
       end
 
+      it 'assigns max_access_for_group' do
+        allow(controller).to receive(:current_user).and_return(user)
+
+        get :index, params: { group_id: group }
+
+        expect(user.max_access_for_group[group.id]).to eq(Gitlab::Access::OWNER)
+      end
+
       it 'assigns invited members' do
         get :index, params: { group_id: group }
 
@@ -65,8 +72,8 @@ RSpec.describe Groups::GroupMembersController do
     end
 
     context 'when user has owner access to subgroup' do
-      let(:nested_group) { create(:group, parent: group) }
-      let(:nested_group_user) { create(:user) }
+      let_it_be(:nested_group) { create(:group, parent: group) }
+      let_it_be(:nested_group_user) { create(:user) }
 
       before do
         group.add_owner(user)
@@ -95,7 +102,7 @@ RSpec.describe Groups::GroupMembersController do
   end
 
   describe 'POST create' do
-    let(:group_user) { create(:user) }
+    let_it_be(:group_user) { create(:user) }
 
     before do
       sign_in(user)
@@ -189,7 +196,7 @@ RSpec.describe Groups::GroupMembersController do
   end
 
   describe 'PUT update' do
-    let(:requester) { create(:group_member, :access_request, group: group) }
+    let_it_be(:requester) { create(:group_member, :access_request, group: group) }
 
     before do
       group.add_owner(user)
@@ -292,9 +299,9 @@ RSpec.describe Groups::GroupMembersController do
   end
 
   describe 'DELETE destroy' do
-    let(:sub_group) { create(:group, parent: group) }
-    let!(:member) { create(:group_member, :developer, group: group) }
-    let!(:sub_member) { create(:group_member, :developer, group: sub_group, user: member.user) }
+    let_it_be(:sub_group) { create(:group, parent: group) }
+    let_it_be(:member) { create(:group_member, :developer, group: group) }
+    let_it_be(:sub_member) { create(:group_member, :developer, group: sub_group, user: member.user) }
 
     before do
       sign_in(user)
@@ -403,6 +410,8 @@ RSpec.describe Groups::GroupMembersController do
       end
 
       context 'and is a requester' do
+        let(:group) { create(:group, :public) }
+
         before do
           group.request_access(user)
         end
@@ -435,7 +444,7 @@ RSpec.describe Groups::GroupMembersController do
   end
 
   describe 'POST approve_access_request' do
-    let(:member) { create(:group_member, :access_request, group: group) }
+    let_it_be(:member) { create(:group_member, :access_request, group: group) }
 
     before do
       sign_in(user)
@@ -479,6 +488,8 @@ RSpec.describe Groups::GroupMembersController do
   end
 
   context 'with external authorization enabled' do
+    let_it_be(:membership) { create(:group_member, group: group) }
+
     before do
       enable_external_authorization_service_check
       group.add_owner(user)

spec/models/group_spec.rb

@@ -955,140 +955,167 @@ RSpec.describe Group do
     it { expect(subject.parent).to be_kind_of(described_class) }
   end
 
-  describe '#max_member_access_for_user' do
-    context 'group shared with another group' do
-      let(:parent_group_user) { create(:user) }
-      let(:group_user) { create(:user) }
-      let(:child_group_user) { create(:user) }
-
-      let_it_be(:group_parent) { create(:group, :private) }
-      let_it_be(:group) { create(:group, :private, parent: group_parent) }
-      let_it_be(:group_child) { create(:group, :private, parent: group) }
-
-      let_it_be(:shared_group_parent) { create(:group, :private) }
-      let_it_be(:shared_group) { create(:group, :private, parent: shared_group_parent) }
-      let_it_be(:shared_group_child) { create(:group, :private, parent: shared_group) }
-
-      before do
-        group_parent.add_owner(parent_group_user)
-        group.add_owner(group_user)
-        group_child.add_owner(child_group_user)
-
-        create(:group_group_link, { shared_with_group: group,
-                                    shared_group: shared_group,
-                                    group_access: GroupMember::DEVELOPER })
-      end
+  context "with member access" do
+    let_it_be(:group_user) { create(:user) }
 
+    describe '#max_member_access_for_user' do
       context 'with user in the group' do
-        let(:user) { group_user }
+        before do
+          group.add_owner(group_user)
+        end
 
         it 'returns correct access level' do
-          expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
-          expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER)
-          expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER)
+          expect(group.max_member_access_for_user(group_user)).to eq(Gitlab::Access::OWNER)
         end
+      end
 
-        context 'with lower group access level than max access level for share' do
-          let(:user) { create(:user) }
+      context 'when user is nil' do
+        it 'returns NO_ACCESS' do
+          expect(group.max_member_access_for_user(nil)).to eq(Gitlab::Access::NO_ACCESS)
+        end
+      end
 
-          it 'returns correct access level' do
-            group.add_reporter(user)
+      context 'evaluating admin access level' do
+        let_it_be(:admin) { create(:admin) }
 
-            expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
-            expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER)
-            expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER)
+        context 'when admin mode is enabled', :enable_admin_mode do
+          it 'returns OWNER by default' do
+            expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::OWNER)
           end
         end
-      end
 
-      context 'with user in the parent group' do
-        let(:user) { parent_group_user }
+        context 'when admin mode is disabled' do
+          it 'returns NO_ACCESS' do
+            expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::NO_ACCESS)
+          end
+        end
 
-        it 'returns correct access level' do
-          expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
-          expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
-          expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+        it 'returns NO_ACCESS when only concrete membership should be considered' do
+          expect(group.max_member_access_for_user(admin, only_concrete_membership: true))
+            .to eq(Gitlab::Access::NO_ACCESS)
         end
       end
 
-      context 'with user in the child group' do
-        let(:user) { child_group_user }
+      context 'when max_access_for_group is set' do
+        let(:max_member_access) { 111 }
 
-        it 'returns correct access level' do
-          expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
-          expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
-          expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+        before do
+          group_user.max_access_for_group[group.id] = max_member_access
         end
-      end
-
-      context 'unrelated project owner' do
-        let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 }
-        let!(:group) { create(:group, id: common_id) }
-        let!(:unrelated_project) { create(:project, id: common_id) }
-        let(:user) { unrelated_project.owner }
 
-        it 'returns correct access level' do
-          expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
-          expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
-          expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+        it 'uses the cached value' do
+          expect(group.max_member_access_for_user(group_user)).to eq(max_member_access)
         end
       end
+    end
 
-      context 'user without accepted access request' do
-        let!(:user) { create(:user) }
+    describe '#max_member_access' do
+      context 'group shared with another group' do
+        let_it_be(:parent_group_user) { create(:user) }
+        let_it_be(:child_group_user) { create(:user) }
+
+        let_it_be(:group_parent) { create(:group, :private) }
+        let_it_be(:group) { create(:group, :private, parent: group_parent) }
+        let_it_be(:group_child) { create(:group, :private, parent: group) }
+
+        let_it_be(:shared_group_parent) { create(:group, :private) }
+        let_it_be(:shared_group) { create(:group, :private, parent: shared_group_parent) }
+        let_it_be(:shared_group_child) { create(:group, :private, parent: shared_group) }
 
         before do
-          create(:group_member, :developer, :access_request, user: user, group: group)
+          group_parent.add_owner(parent_group_user)
+          group.add_owner(group_user)
+          group_child.add_owner(child_group_user)
+
+          create(:group_group_link, { shared_with_group: group,
+                                      shared_group: shared_group,
+                                      group_access: GroupMember::DEVELOPER })
         end
 
-        it 'returns correct access level' do
-          expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
-          expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
-          expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+        context 'with user in the group' do
+          it 'returns correct access level' do
+            expect(shared_group_parent.max_member_access(group_user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group.max_member_access(group_user)).to eq(Gitlab::Access::DEVELOPER)
+            expect(shared_group_child.max_member_access(group_user)).to eq(Gitlab::Access::DEVELOPER)
+          end
+
+          context 'with lower group access level than max access level for share' do
+            let(:user) { create(:user) }
+
+            it 'returns correct access level' do
+              group.add_reporter(user)
+
+              expect(shared_group_parent.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
+              expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::REPORTER)
+              expect(shared_group_child.max_member_access(user)).to eq(Gitlab::Access::REPORTER)
+            end
+          end
         end
-      end
-    end
 
-    context 'multiple groups shared with group' do
-      let(:user) { create(:user) }
-      let(:group) { create(:group, :private) }
-      let(:shared_group_parent) { create(:group, :private) }
-      let(:shared_group) { create(:group, :private, parent: shared_group_parent) }
+        context 'with user in the parent group' do
+          it 'returns correct access level' do
+            expect(shared_group_parent.max_member_access(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group.max_member_access(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group_child.max_member_access(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS)
+          end
+        end
 
-      before do
-        group.add_owner(user)
+        context 'with user in the child group' do
+          it 'returns correct access level' do
+            expect(shared_group_parent.max_member_access(child_group_user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group.max_member_access(child_group_user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group_child.max_member_access(child_group_user)).to eq(Gitlab::Access::NO_ACCESS)
+          end
+        end
 
-        create(:group_group_link, { shared_with_group: group,
-                                    shared_group: shared_group,
-                                    group_access: GroupMember::DEVELOPER })
-        create(:group_group_link, { shared_with_group: group,
-                                    shared_group: shared_group_parent,
-                                    group_access: GroupMember::MAINTAINER })
-      end
+        context 'unrelated project owner' do
+          let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 }
+          let!(:group) { create(:group, id: common_id) }
+          let!(:unrelated_project) { create(:project, id: common_id) }
+          let(:user) { unrelated_project.owner }
 
-      it 'returns correct access level' do
-        expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::MAINTAINER)
-      end
-    end
+          it 'returns correct access level' do
+            expect(shared_group_parent.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group_child.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
+          end
+        end
 
-    context 'evaluating admin access level' do
-      let_it_be(:admin) { create(:admin) }
+        context 'user without accepted access request' do
+          let!(:user) { create(:user) }
 
-      context 'when admin mode is enabled', :enable_admin_mode do
-        it 'returns OWNER by default' do
-          expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::OWNER)
+          before do
+            create(:group_member, :developer, :access_request, user: user, group: group)
+          end
+
+          it 'returns correct access level' do
+            expect(shared_group_parent.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group_child.max_member_access(user)).to eq(Gitlab::Access::NO_ACCESS)
+          end
         end
       end
 
-      context 'when admin mode is disabled' do
-        it 'returns NO_ACCESS' do
-          expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::NO_ACCESS)
+      context 'multiple groups shared with group' do
+        let(:user) { create(:user) }
+        let(:group) { create(:group, :private) }
+        let(:shared_group_parent) { create(:group, :private) }
+        let(:shared_group) { create(:group, :private, parent: shared_group_parent) }
+
+        before do
+          group.add_owner(user)
+
+          create(:group_group_link, { shared_with_group: group,
+                                      shared_group: shared_group,
+                                      group_access: GroupMember::DEVELOPER })
+          create(:group_group_link, { shared_with_group: group,
+                                      shared_group: shared_group_parent,
+                                      group_access: GroupMember::MAINTAINER })
         end
-      end
 
-      it 'returns NO_ACCESS when only concrete membership should be considered' do
-        expect(group.max_member_access_for_user(admin, only_concrete_membership: true))
-          .to eq(Gitlab::Access::NO_ACCESS)
+        it 'returns correct access level' do
+          expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::MAINTAINER)
+        end
       end
     end
   end