kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Merge branch 'sh-fix-project-members-api-perf' into 'master'

Browse source 
Remove N+1 queries with /projects/:project_id/{access_requests,members} API endpoints

See merge request gitlab-org/gitlab-ce!16751
This commit is contained in:
Rémy Coutable 2018-01-31 11:35:56 +00:00
commit 08e013431a
6 changed files with 46 additions and 30 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
changelogs/unreleased/sh-fix-project-members-api-perf.yml Normal file
View file

@ -0,0 +1,6 @@
---
title: Remove N+1 queries with /projects/:project_id/{access_requests,members} API
endpoints
merge_request:
author:
type: performance

6
lib/api/access_requests.rb
View file

@ -24,7 +24,7 @@ module API
access_requesters = AccessRequestsFinder.new(source).execute!(current_user)
access_requesters = paginate(access_requesters.includes(:user))
present access_requesters.map(&:user), with: Entities::AccessRequester, source: source
present access_requesters, with: Entities::AccessRequester
end
desc "Requests access for the authenticated user to a #{source_type}." do
@ -36,7 +36,7 @@ module API
access_requester = source.request_access(current_user)
if access_requester.persisted?
present access_requester.user, with: Entities::AccessRequester, access_requester: access_requester
present access_requester, with: Entities::AccessRequester
else
render_validation_error!(access_requester)
end
@ -56,7 +56,7 @@ module API
member = ::Members::ApproveAccessRequestService.new(source, current_user, declared_params).execute
status :created
present member.user, with: Entities::Member, member: member
present member, with: Entities::Member
end
desc 'Denies an access request for the given user.' do

21
lib/api/entities.rb
View file

@ -205,22 +205,15 @@ module API
expose :build_artifacts_size, as: :job_artifacts_size
end
class Member < UserBasic
expose :access_level do |user, options|
member = options[:member] || options[:source].members.find_by(user_id: user.id)
member.access_level
end
expose :expires_at do |user, options|
member = options[:member] || options[:source].members.find_by(user_id: user.id)
member.expires_at
end
class Member < Grape::Entity
expose :user, merge: true, using: UserBasic
expose :access_level
expose :expires_at
end
class AccessRequester < UserBasic
expose :requested_at do |user, options|
access_requester = options[:access_requester] || options[:source].requesters.find_by(user_id: user.id)
access_requester.requested_at
end
class AccessRequester < Grape::Entity
expose :user, merge: true, using: UserBasic
expose :requested_at
end
class Group < Grape::Entity

13
lib/api/members.rb
View file

@ -21,10 +21,11 @@ module API
get ":id/members" do
source = find_source(source_type, params[:id])
users = source.users
users = users.merge(User.search(params[:query])) if params[:query].present?
members = source.members.where.not(user_id: nil).includes(:user)
members = members.joins(:user).merge(User.search(params[:query])) if params[:query].present?
members = paginate(members)
present paginate(users), with: Entities::Member, source: source
present members, with: Entities::Member
end
desc 'Gets a member of a group or project.' do
@ -39,7 +40,7 @@ module API
members = source.members
member = members.find_by!(user_id: params[:user_id])
present member.user, with: Entities::Member, member: member
present member, with: Entities::Member
end
desc 'Adds a member to a group or project.' do
@ -62,7 +63,7 @@ module API
if !member
not_allowed! # This currently can only be reached in EE
elsif member.persisted? && member.valid?
present member.user, with: Entities::Member, member: member
present member, with: Entities::Member
else
render_validation_error!(member)
end
@ -83,7 +84,7 @@ module API
member = source.members.find_by!(user_id: params.delete(:user_id))
if member.update_attributes(declared_params(include_missing: false))
present member.user, with: Entities::Member, member: member
present member, with: Entities::Member
else
render_validation_error!(member)
end

15
lib/api/v3/members.rb
View file

@ -22,10 +22,11 @@ module API
get ":id/members" do
source = find_source(source_type, params[:id])
users = source.users
users = users.merge(User.search(params[:query])) if params[:query].present?
members = source.members.where.not(user_id: nil).includes(:user)
members = members.joins(:user).merge(User.search(params[:query])) if params[:query].present?
members = paginate(members)
present paginate(users), with: ::API::Entities::Member, source: source
present members, with: ::API::Entities::Member
end
desc 'Gets a member of a group or project.' do
@ -40,7 +41,7 @@ module API
members = source.members
member = members.find_by!(user_id: params[:user_id])
present member.user, with: ::API::Entities::Member, member: member
present member, with: ::API::Entities::Member
end
desc 'Adds a member to a group or project.' do
@ -69,7 +70,7 @@ module API
end
if member.persisted? && member.valid?
present member.user, with: ::API::Entities::Member, member: member
present member, with: ::API::Entities::Member
else
# This is to ensure back-compatibility but 400 behavior should be used
# for all validation errors in 9.0!
@ -93,7 +94,7 @@ module API
member = source.members.find_by!(user_id: params.delete(:user_id))
if member.update_attributes(declared_params(include_missing: false))
present member.user, with: ::API::Entities::Member, member: member
present member, with: ::API::Entities::Member
else
# This is to ensure back-compatibility but 400 behavior should be used
# for all validation errors in 9.0!
@ -125,7 +126,7 @@ module API
else
::Members::DestroyService.new(source, current_user, declared_params).execute
present member.user, with: ::API::Entities::Member, member: member
present member, with: ::API::Entities::Member
end
end
end

15
spec/requests/api/members_spec.rb
View file

@ -44,6 +44,21 @@ describe API::Members do
end
end
it 'avoids N+1 queries' do
# Establish baseline
get api("/#{source_type.pluralize}/#{source.id}/members", master)
control = ActiveRecord::QueryRecorder.new do
get api("/#{source_type.pluralize}/#{source.id}/members", master)
end
project.add_developer(create(:user))
expect do
get api("/#{source_type.pluralize}/#{source.id}/members", master)
end.not_to exceed_query_limit(control)
end
it 'does not return invitees' do
create(:"#{source_type}_member", invite_token: '123', invite_email: 'test@abc.com', source: source, user: nil)