Switch group member max access to use bulk load

- already created paradigm and exactly what we want.
- prevents duplication...has a much larger impact.

Changelog: performance

app/controllers/groups/group_members_controller.rb

@@ -25,7 +25,6 @@ class Groups::GroupMembersController < Groups::ApplicationController
   helper_method :can_manage_members?
 
   def index
-    preload_max_access
     @sort = params[:sort].presence || sort_value_name
 
     @members = GroupMembersFinder
@@ -54,14 +53,6 @@ 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?
     strong_memoize(:can_manage_members) do
       can?(current_user, :admin_group_member, @group)

app/models/group.rb

@@ -17,6 +17,7 @@ class Group < Namespace
   include GroupAPICompatibility
   include EachBatch
   include HasTimelogsReport
+  include BulkMemberAccessLoad
 
   ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT = 10
 
@@ -569,24 +570,8 @@ 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
-    # Use the preloaded value that exists instead of performing the db query again(cached or not).
-    # Groups::GroupMembersController#preload_max_access makes use of this by
-    # calling Group#max_member_access. This helps when we have a process
-    # that may query this multiple times from the outside through a policy query
-    # like the GroupPolicy#lookup_access_level! does as a condition for any role
-    return user.max_access_for_group[id] if user.max_access_for_group[id]
 
-    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
+    max_member_access([user.id])[user.id]
   end
 
   def mattermost_team_params
@@ -730,6 +715,12 @@ class Group < Namespace
 
   private
 
+  def max_member_access(user_ids)
+    max_member_access_for_resource_ids(User, user_ids) do |user_ids|
+      members_with_parents.where(user_id: user_ids).group(:user_id).maximum(:access_level)
+    end
+  end
+
   def update_two_factor_requirement
     return unless saved_change_to_require_two_factor_authentication? || saved_change_to_two_factor_grace_period?
 

app/models/user.rb

@@ -99,12 +99,6 @@ 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
   #

ee/spec/controllers/groups/group_members_controller_spec.rb

@@ -20,15 +20,15 @@ RSpec.describe Groups::GroupMembersController do
       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
+      it 'records queries', :request_store, :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, group: group, created_by: user)
         create_list(:group_member, 5, :access_request, group: group)
-        # 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
+        # locally 39 vs 43 GDK vs 48 CI
+        unresolved_n_plus_ones = 4 # still have a few queries created by can_update/can_remove that could be reduced
         multiple_members_threshold = 5 # GDK vs CI difference
 
         expect do

spec/controllers/groups/group_members_controller_spec.rb

@@ -32,14 +32,6 @@ RSpec.describe Groups::GroupMembersController do
         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 }
 

spec/models/group_spec.rb

@@ -1093,167 +1093,151 @@ RSpec.describe Group do
     it { expect(subject.parent).to be_kind_of(described_class) }
   end
 
-  context "with member access" do
+  describe '#max_member_access_for_user' do
     let_it_be(:group_user) { create(:user) }
 
-    describe '#max_member_access_for_user' do
-      context 'with user in the group' do
-        before do
-          group.add_owner(group_user)
-        end
-
-        it 'returns correct access level' do
-          expect(group.max_member_access_for_user(group_user)).to eq(Gitlab::Access::OWNER)
-        end
+    context 'with user in the group' do
+      before do
+        group.add_owner(group_user)
       end
 
-      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
+      it 'returns correct access level' do
+        expect(group.max_member_access_for_user(group_user)).to eq(Gitlab::Access::OWNER)
       end
+    end
 
-      context 'evaluating admin access level' do
-        let_it_be(:admin) { create(:admin) }
-
-        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
+    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
 
-        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
+    context 'evaluating admin access level' do
+      let_it_be(:admin) { create(:admin) }
 
-        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)
+      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
 
-      context 'when max_access_for_group is set' do
-        let(:max_member_access) { 111 }
-
-        before do
-          group_user.max_access_for_group[group.id] = max_member_access
+      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 'uses the cached value' do
-          expect(group.max_member_access_for_user(group_user)).to eq(max_member_access)
-        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)
       end
     end
 
-    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) }
+    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(: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) }
+      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)
+      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
+        create(:group_group_link, { shared_with_group: group,
+                                    shared_group: shared_group,
+                                    group_access: GroupMember::DEVELOPER })
+      end
 
-        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 user in the group' do
+        it 'returns correct access level' do
+          expect(shared_group_parent.max_member_access_for_user(group_user)).to eq(Gitlab::Access::NO_ACCESS)
+          expect(shared_group.max_member_access_for_user(group_user)).to eq(Gitlab::Access::DEVELOPER)
+          expect(shared_group_child.max_member_access_for_user(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) }
+        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)
+          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
+            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)
           end
         end
+      end
 
-        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
+      context 'with user in the parent group' do
+        it 'returns correct access level' do
+          expect(shared_group_parent.max_member_access_for_user(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS)
+          expect(shared_group.max_member_access_for_user(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS)
+          expect(shared_group_child.max_member_access_for_user(parent_group_user)).to eq(Gitlab::Access::NO_ACCESS)
         end
+      end
 
-        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
+      context 'with user in the child group' do
+        it 'returns correct access level' do
+          expect(shared_group_parent.max_member_access_for_user(child_group_user)).to eq(Gitlab::Access::NO_ACCESS)
+          expect(shared_group.max_member_access_for_user(child_group_user)).to eq(Gitlab::Access::NO_ACCESS)
+          expect(shared_group_child.max_member_access_for_user(child_group_user)).to eq(Gitlab::Access::NO_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 }
+      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(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
+        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)
         end
+      end
 
-        context 'user without accepted access request' do
-          let!(:user) { create(:user) }
+      context 'user without accepted access request' do
+        let!(:user) { create(:user) }
 
-          before do
-            create(:group_member, :developer, :access_request, user: user, group: group)
-          end
+        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
+        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)
         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 '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)
+      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
+        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
 
-        it 'returns correct access level' do
-          expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::MAINTAINER)
-        end
+      it 'returns correct access level' do
+        expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::MAINTAINER)
       end
     end
   end