Skip to content

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?

  1. app/views/groups/group_members/index.html.haml calls group_members_list_data_attributes (in a few places).
  2. Groups::GroupMembersHelper#group_members_list_data_attributes calls members_data_json.
  3. Groups::GroupMembersHelper#members_data_json calls MemberSerializer.
  4. MemberSerializer uses the MemberEntity.
  5. MemberEntity exposes using_license, can_update and can_remove, which are invoked in the Vue app through the app/assets/javascripts/members/utils.js file.
  6. 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 and 30+ query improvement(w/o cache) in our controller test since there are many items in this controller action eventually going through the group policy.
  7. can_update? or can_remove? is called, they similarly go through the GroupPolicy via this rule.

Why does this fix the n+1?

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

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

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

Edited by Doug Stull

Merge request reports