Merge branch '329587-reuse-bulkmemberaccessload-for-group' into 'master'

Reuse BulkMemberAccessLoad on groups [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!62226

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,10 +1093,9 @@ 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)
@@ -1134,20 +1133,6 @@ RSpec.describe Group do
       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
-        end
-
-        it 'uses the cached value' do
-          expect(group.max_member_access_for_user(group_user)).to eq(max_member_access)
-        end
-      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) }
@@ -1172,9 +1157,9 @@ RSpec.describe Group do
 
       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)
+          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
@@ -1183,26 +1168,26 @@ RSpec.describe Group do
           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)
+            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)
+          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)
+          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
 
@@ -1213,9 +1198,9 @@ RSpec.describe Group do
         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)
+          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
 
@@ -1227,9 +1212,9 @@ RSpec.describe Group do
         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)
+          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
@@ -1252,8 +1237,7 @@ RSpec.describe Group do
       end
 
       it 'returns correct access level' do
-          expect(shared_group.max_member_access(user)).to eq(Gitlab::Access::MAINTAINER)
-        end
+        expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::MAINTAINER)
       end
     end
   end