Commit 66cfb901 authored by Rubén Dávila's avatar Rubén Dávila

Optimize SQL queries used in Groups::GroupMembersController#create

The following optimizations were performed:

- Add new association to GroupMember and ProjectMember

  This new association will allow us to check if a user is a member of a
  Project or Group through a single query instead of two.

- Optimize retrieving of Members when adding multiple Users
parent b8b49938
...@@ -16,6 +16,7 @@ class Group < Namespace ...@@ -16,6 +16,7 @@ class Group < Namespace
source: :user source: :user
has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent has_many :requesters, -> { where.not(requested_at: nil) }, dependent: :destroy, as: :source, class_name: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent
has_many :members_and_requesters, as: :source, class_name: 'GroupMember'
has_many :milestones has_many :milestones
has_many :project_group_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :project_group_links, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
......
...@@ -126,20 +126,11 @@ class Member < ActiveRecord::Base ...@@ -126,20 +126,11 @@ class Member < ActiveRecord::Base
find_by(invite_token: invite_token) find_by(invite_token: invite_token)
end end
def add_user(source, user, access_level, current_user: nil, expires_at: nil) def add_user(source, user, access_level, existing_members: nil, current_user: nil, expires_at: nil)
user = retrieve_user(user) # `user` can be either a User object, User ID or an email to be invited
member = retrieve_member(source, user, existing_members)
access_level = retrieve_access_level(access_level) access_level = retrieve_access_level(access_level)
# `user` can be either a User object or an email to be invited
member =
if user.is_a?(User)
source.members.find_by(user_id: user.id) ||
source.requesters.find_by(user_id: user.id) ||
source.members.build(user_id: user.id)
else
source.members.build(invite_email: user)
end
return member unless can_update_member?(current_user, member) return member unless can_update_member?(current_user, member)
member.attributes = { member.attributes = {
...@@ -165,17 +156,15 @@ class Member < ActiveRecord::Base ...@@ -165,17 +156,15 @@ class Member < ActiveRecord::Base
def add_users(source, users, access_level, current_user: nil, expires_at: nil) def add_users(source, users, access_level, current_user: nil, expires_at: nil)
return [] unless users.present? return [] unless users.present?
# Collect all user ids into separate array emails, users, existing_members = parse_users_list(source, users)
# so we can use single sql query to get user objects
user_ids = users.select { |user| user =~ /\A\d+\Z/ }
users = users - user_ids + User.where(id: user_ids)
self.transaction do self.transaction do
users.map do |user| (emails + users).map! do |user|
add_user( add_user(
source, source,
user, user,
access_level, access_level,
existing_members: existing_members,
current_user: current_user, current_user: current_user,
expires_at: expires_at expires_at: expires_at
) )
...@@ -189,6 +178,31 @@ class Member < ActiveRecord::Base ...@@ -189,6 +178,31 @@ class Member < ActiveRecord::Base
private private
def parse_users_list(source, list)
emails, user_ids, users = [], [], []
existing_members = {}
list.each do |item|
case item
when User
users << item
when Integer
user_ids << item
when /\A\d+\Z/
user_ids << item.to_i
when Devise.email_regexp
emails << item
end
end
if user_ids.present?
users.concat(User.where(id: user_ids))
existing_members = source.members_and_requesters.where(user_id: user_ids).index_by(&:user_id)
end
[emails, users, existing_members]
end
# This method is used to find users that have been entered into the "Add members" field. # This method is used to find users that have been entered into the "Add members" field.
# These can be the User objects directly, their IDs, their emails, or new emails to be invited. # These can be the User objects directly, their IDs, their emails, or new emails to be invited.
def retrieve_user(user) def retrieve_user(user)
...@@ -197,6 +211,20 @@ class Member < ActiveRecord::Base ...@@ -197,6 +211,20 @@ class Member < ActiveRecord::Base
User.find_by(id: user) || User.find_by(email: user) || user User.find_by(id: user) || User.find_by(email: user) || user
end end
def retrieve_member(source, user, existing_members)
user = retrieve_user(user)
if user.is_a?(User)
if existing_members
existing_members[user.id] || source.members.build(user_id: user.id)
else
source.members_and_requesters.find_or_initialize_by(user_id: user.id)
end
else
source.members.build(invite_email: user)
end
end
def retrieve_access_level(access_level) def retrieve_access_level(access_level)
access_levels.fetch(access_level) { access_level.to_i } access_levels.fetch(access_level) { access_level.to_i }
end end
......
...@@ -144,6 +144,7 @@ class Project < ActiveRecord::Base ...@@ -144,6 +144,7 @@ class Project < ActiveRecord::Base
has_many :requesters, -> { where.not(requested_at: nil) }, has_many :requesters, -> { where.not(requested_at: nil) },
as: :source, class_name: 'ProjectMember', dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent as: :source, class_name: 'ProjectMember', dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
has_many :members_and_requesters, as: :source, class_name: 'ProjectMember'
has_many :deploy_keys_projects has_many :deploy_keys_projects
has_many :deploy_keys, through: :deploy_keys_projects has_many :deploy_keys, through: :deploy_keys_projects
......
...@@ -264,6 +264,7 @@ project: ...@@ -264,6 +264,7 @@ project:
- statistics - statistics
- container_repositories - container_repositories
- uploads - uploads
- members_and_requesters
award_emoji: award_emoji:
- awardable - awardable
- user - user
......
...@@ -9,6 +9,7 @@ describe Group do ...@@ -9,6 +9,7 @@ describe Group do
it { is_expected.to have_many(:users).through(:group_members) } it { is_expected.to have_many(:users).through(:group_members) }
it { is_expected.to have_many(:owners).through(:group_members) } it { is_expected.to have_many(:owners).through(:group_members) }
it { is_expected.to have_many(:requesters).dependent(:destroy) } it { is_expected.to have_many(:requesters).dependent(:destroy) }
it { is_expected.to have_many(:members_and_requesters) }
it { is_expected.to have_many(:project_group_links).dependent(:destroy) } it { is_expected.to have_many(:project_group_links).dependent(:destroy) }
it { is_expected.to have_many(:shared_projects).through(:project_group_links) } it { is_expected.to have_many(:shared_projects).through(:project_group_links) }
it { is_expected.to have_many(:notification_settings).dependent(:destroy) } it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
...@@ -25,22 +26,8 @@ describe Group do ...@@ -25,22 +26,8 @@ describe Group do
group.add_developer(developer) group.add_developer(developer)
end end
describe '#members' do it_behaves_like 'members and requesters associations' do
it 'includes members and exclude requesters' do let(:namespace) { group }
member_user_ids = group.members.pluck(:user_id)
expect(member_user_ids).to include(developer.id)
expect(member_user_ids).not_to include(requester.id)
end
end
describe '#requesters' do
it 'does not include requesters' do
requester_user_ids = group.requesters.pluck(:user_id)
expect(requester_user_ids).to include(requester.id)
expect(requester_user_ids).not_to include(developer.id)
end
end end
end end
end end
......
...@@ -409,6 +409,15 @@ describe Member do ...@@ -409,6 +409,15 @@ describe Member do
expect(members).to be_a Array expect(members).to be_a Array
expect(members).to be_empty expect(members).to be_empty
end end
it 'supports differents formats' do
list = ['joe@local.test', admin, user1.id, user2.id.to_s]
members = described_class.add_users(source, list, :master)
expect(members.size).to eq(4)
expect(members.first).to be_invite
end
end end
end end
end end
......
...@@ -74,6 +74,7 @@ describe Project do ...@@ -74,6 +74,7 @@ describe Project do
it { is_expected.to have_many(:forks).through(:forked_project_links) } it { is_expected.to have_many(:forks).through(:forked_project_links) }
it { is_expected.to have_many(:uploads).dependent(:destroy) } it { is_expected.to have_many(:uploads).dependent(:destroy) }
it { is_expected.to have_many(:pipeline_schedules) } it { is_expected.to have_many(:pipeline_schedules) }
it { is_expected.to have_many(:members_and_requesters) }
context 'after initialized' do context 'after initialized' do
it "has a project_feature" do it "has a project_feature" do
...@@ -90,22 +91,8 @@ describe Project do ...@@ -90,22 +91,8 @@ describe Project do
project.team << [developer, :developer] project.team << [developer, :developer]
end end
describe '#members' do it_behaves_like 'members and requesters associations' do
it 'includes members and exclude requesters' do let(:namespace) { project }
member_user_ids = project.members.pluck(:user_id)
expect(member_user_ids).to include(developer.id)
expect(member_user_ids).not_to include(requester.id)
end
end
describe '#requesters' do
it 'does not include requesters' do
requester_user_ids = project.requesters.pluck(:user_id)
expect(requester_user_ids).to include(requester.id)
expect(requester_user_ids).not_to include(developer.id)
end
end end
end end
......
RSpec.shared_examples 'members and requesters associations' do
describe '#members_and_requesters' do
it 'includes members and requesters' do
member_and_requester_user_ids = namespace.members_and_requesters.pluck(:user_id)
expect(member_and_requester_user_ids).to include(requester.id, developer.id)
end
end
describe '#members' do
it 'includes members and exclude requesters' do
member_user_ids = namespace.members.pluck(:user_id)
expect(member_user_ids).to include(developer.id)
expect(member_user_ids).not_to include(requester.id)
end
end
describe '#requesters' do
it 'does not include requesters' do
requester_user_ids = namespace.requesters.pluck(:user_id)
expect(requester_user_ids).to include(requester.id)
expect(requester_user_ids).not_to include(developer.id)
end
end
end
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