Resolve admin_group_member group policy n+1
What does this MR do?
Improves the overall query count and timing when loading Groups::GroupMembersController#index
as part of the effort to resolve https://gitlab.com/gitlab-org/gitlab/-/issues/21033
- Resolves n+1 query issues on checking GroupMember/Group policy when using the MemberSerializer.
What did this do before?
-
app/views/groups/group_members/index.html.haml
calls group_members_list_data_attributes (in a few places). -
Groups::GroupMembersHelper#group_members_list_data_attributes
calls members_data_json. -
Groups::GroupMembersHelper#members_data_json
calls MemberSerializer. -
MemberSerializer
uses the MemberEntity. -
MemberEntity
exposes using_license, can_update and can_remove, which are invoked in the Vue app through theapp/assets/javascripts/members/utils.js
file. - When
using_license
is called, it triggers a condition in GroupPolicy where the owner condition is called, which invokes access_level -> lookup_access_level! -> group.max_member_access_for_user on the current user for every single member.- Note, this may end up getting some help from cache, but still adds the call to the queries and shows a
n+1
and30+
query improvement(w/o cache) in our controller test since there are many items in this controller action eventually going through the group policy.
- Note, this may end up getting some help from cache, but still adds the call to the queries and shows a
-
can_update?
orcan_remove?
is called, they similarly go through the GroupPolicy via this rule.
n+1
?
Why does this fix the As part of the modification to Group#max_member_access_for_user
, we allow the methods to return early if the virtual attributes that we set on the member have been set. This avoids the n+1
query while still allowing our original functionality to remain.
Performance analysis locally on GDK
Caveat This was done on a group where there were a handful of members(10), some of them being invited(45), and Access Requests(2). The numbers and query times will vary based upon size.
For a user who can't admin users
Before(after gdk restart and cache warm-up)
- ActiveRecord Query time:
~95ms
- queries(cached/uncached):
54
After(after gdk restart and cache warm-up)
- ActiveRecord Query time:
~56ms
- queries(cached/uncached):
36
For a user who can admin users as a direct owner
Before(after gdk restart and cache warm-up)
- ActiveRecord Query time:
~134ms
- queries(cached/uncached):
79
After(after gdk restart and cache warm-up)
- ActiveRecord Query time:
~87ms
- queries(cached/uncached):
56
For a user who is instance admin
- no net change due to early return
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Related to #21033