Commit 79c6b9ca authored by Stan Hu's avatar Stan Hu
Browse files

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

We can simplify the code quite a bit and improve performance by using
grape-entity merge fields:

https://github.com/ruby-grape/grape-entity/tree/v0.6.0#merge-fields

Relates to #42030
parent 419029fb5f24
---
title: Remove N+1 queries with /projects/:project_id/{access_requests,members} API
endpoints
merge_request:
author:
type: performance
......@@ -24,7 +24,7 @@ class AccessRequests < Grape::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 @@ class AccessRequests < Grape::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 @@ class AccessRequests < Grape::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
......
......@@ -205,22 +205,15 @@ class ProjectStatistics < Grape::Entity
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
......
......@@ -21,10 +21,11 @@ class Members < Grape::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 @@ class Members < Grape::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 @@ class Members < Grape::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 @@ class Members < Grape::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
......
......@@ -22,10 +22,11 @@ class Members < Grape::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 @@ class Members < Grape::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 @@ class Members < Grape::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 @@ class Members < Grape::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 @@ class Members < Grape::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
......
......@@ -44,6 +44,21 @@
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)
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment