Commit c5f9b2be authored by Sean McGivern's avatar Sean McGivern

Remove N+1 queries from users autocomplete

Both of these were related to groups:

1. We need to preload routes (using the `with_route` scope) if we're
   going to get the group's path.
2. We were counting each group's members separately.

They're in the same commit because the spec for N+1 detection wouldn't
pass with only one of these fixes.
parent 05c6de98
...@@ -14,6 +14,8 @@ class GroupMember < Member ...@@ -14,6 +14,8 @@ class GroupMember < Member
scope :in_groups, ->(groups) { where(source_id: groups.select(:id)) } scope :in_groups, ->(groups) { where(source_id: groups.select(:id)) }
scope :count_users_by_group_id, -> { joins(:user).group(:source_id).count }
after_create :update_two_factor_requirement, unless: :invite? after_create :update_two_factor_requirement, unless: :invite?
after_destroy :update_two_factor_requirement, unless: :invite? after_destroy :update_two_factor_requirement, unless: :invite?
......
...@@ -28,19 +28,35 @@ module Users ...@@ -28,19 +28,35 @@ module Users
end end
def groups def groups
current_user.authorized_groups.sort_by(&:path).map do |group| group_counts = GroupMember
group_as_hash(group) .in_groups(current_user.authorized_groups)
.non_request
.count_users_by_group_id
current_user.authorized_groups.with_route.sort_by(&:path).map do |group|
group_as_hash(group, group_counts)
end end
end end
private private
def user_as_hash(user) def user_as_hash(user)
{ type: user.class.name, username: user.username, name: user.name, avatar_url: user.avatar_url } {
type: user.class.name,
username: user.username,
name: user.name,
avatar_url: user.avatar_url
}
end end
def group_as_hash(group) def group_as_hash(group, group_counts)
{ type: group.class.name, username: group.full_path, name: group.full_name, avatar_url: group.avatar_url, count: group.users.count } {
type: group.class.name,
username: group.full_path,
name: group.full_name,
avatar_url: group.avatar_url,
count: group_counts.fetch(group.id, 0)
}
end end
end end
end end
---
title: Fix some N+1s in loading routes and counting members for groups in @-autocomplete
merge_request: 26491
author:
type: performance
require 'spec_helper' require 'spec_helper'
describe GroupMember do describe GroupMember do
describe '.count_users_by_group_id' do
it 'counts users by group ID' do
user_1 = create(:user)
user_2 = create(:user)
group_1 = create(:group)
group_2 = create(:group)
group_1.add_owner(user_1)
group_1.add_owner(user_2)
group_2.add_owner(user_1)
expect(described_class.count_users_by_group_id).to eq(group_1.id => 2,
group_2.id => 1)
end
end
describe '.access_level_roles' do describe '.access_level_roles' do
it 'returns Gitlab::Access.options_with_owner' do it 'returns Gitlab::Access.options_with_owner' do
expect(described_class.access_level_roles).to eq(Gitlab::Access.options_with_owner) expect(described_class.access_level_roles).to eq(Gitlab::Access.options_with_owner)
......
...@@ -2,29 +2,56 @@ require 'spec_helper' ...@@ -2,29 +2,56 @@ require 'spec_helper'
describe Projects::ParticipantsService do describe Projects::ParticipantsService do
describe '#groups' do describe '#groups' do
set(:user) { create(:user) }
set(:project) { create(:project, :public) }
let(:service) { described_class.new(project, user) }
it 'avoids N+1 queries' do
group_1 = create(:group)
group_1.add_owner(user)
service.groups # Run general application warmup queries
control_count = ActiveRecord::QueryRecorder.new { service.groups }.count
group_2 = create(:group)
group_2.add_owner(user)
expect { service.groups }.not_to exceed_query_limit(control_count)
end
it 'returns correct user counts for groups' do
group_1 = create(:group)
group_1.add_owner(user)
group_1.add_owner(create(:user))
group_2 = create(:group)
group_2.add_owner(user)
create(:group_member, :access_request, group: group_2, user: create(:user))
expect(service.groups).to contain_exactly(
a_hash_including(name: group_1.full_name, count: 2),
a_hash_including(name: group_2.full_name, count: 1)
)
end
describe 'avatar_url' do describe 'avatar_url' do
let(:project) { create(:project, :public) }
let(:group) { create(:group, avatar: fixture_file_upload('spec/fixtures/dk.png')) } let(:group) { create(:group, avatar: fixture_file_upload('spec/fixtures/dk.png')) }
let(:user) { create(:user) }
let!(:group_member) { create(:group_member, group: group, user: user) }
it 'should return an url for the avatar' do before do
participants = described_class.new(project, user) group.add_owner(user)
groups = participants.groups end
expect(groups.size).to eq 1 it 'should return an url for the avatar' do
expect(groups.first[:avatar_url]).to eq("/uploads/-/system/group/avatar/#{group.id}/dk.png") expect(service.groups.size).to eq 1
expect(service.groups.first[:avatar_url]).to eq("/uploads/-/system/group/avatar/#{group.id}/dk.png")
end end
it 'should return an url for the avatar with relative url' do it 'should return an url for the avatar with relative url' do
stub_config_setting(relative_url_root: '/gitlab') stub_config_setting(relative_url_root: '/gitlab')
stub_config_setting(url: Settings.send(:build_gitlab_url)) stub_config_setting(url: Settings.send(:build_gitlab_url))
participants = described_class.new(project, user) expect(service.groups.size).to eq 1
groups = participants.groups expect(service.groups.first[:avatar_url]).to eq("/gitlab/uploads/-/system/group/avatar/#{group.id}/dk.png")
expect(groups.size).to eq 1
expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/-/system/group/avatar/#{group.id}/dk.png")
end end
end 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