From c5f9b2be826c05e5f06d424f5c110976ad0b68c4 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Fri, 22 Mar 2019 15:51:14 +0000
Subject: [PATCH] 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.
---
app/models/members/group_member.rb | 2 +
.../concerns/users/participable_service.rb | 26 +++++++--
...routes-n-plus-one-in-user-autocomplete.yml | 5 ++
spec/models/members/group_member_spec.rb | 16 ++++++
.../projects/participants_service_spec.rb | 53 ++++++++++++++-----
5 files changed, 84 insertions(+), 18 deletions(-)
create mode 100644 changelogs/unreleased/fix-routes-n-plus-one-in-user-autocomplete.yml
diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb
index 2c9e1ba1d80d6..510f856087dc6 100644
--- a/app/models/members/group_member.rb
+++ b/app/models/members/group_member.rb
@@ -14,6 +14,8 @@ class GroupMember < Member
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_destroy :update_two_factor_requirement, unless: :invite?
diff --git a/app/services/concerns/users/participable_service.rb b/app/services/concerns/users/participable_service.rb
index 6713b6617aecc..a3cc6014fd3a4 100644
--- a/app/services/concerns/users/participable_service.rb
+++ b/app/services/concerns/users/participable_service.rb
@@ -28,19 +28,35 @@ def sorted(users)
end
def groups
- current_user.authorized_groups.sort_by(&:path).map do |group|
- group_as_hash(group)
+ group_counts = GroupMember
+ .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
private
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
- def group_as_hash(group)
- { type: group.class.name, username: group.full_path, name: group.full_name, avatar_url: group.avatar_url, count: group.users.count }
+ 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_counts.fetch(group.id, 0)
+ }
end
end
end
diff --git a/changelogs/unreleased/fix-routes-n-plus-one-in-user-autocomplete.yml b/changelogs/unreleased/fix-routes-n-plus-one-in-user-autocomplete.yml
new file mode 100644
index 0000000000000..ae097e859d95f
--- /dev/null
+++ b/changelogs/unreleased/fix-routes-n-plus-one-in-user-autocomplete.yml
@@ -0,0 +1,5 @@
+---
+title: Fix some N+1s in loading routes and counting members for groups in @-autocomplete
+merge_request: 26491
+author:
+type: performance
diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb
index a3451c67bd864..bc937368cff56 100644
--- a/spec/models/members/group_member_spec.rb
+++ b/spec/models/members/group_member_spec.rb
@@ -1,6 +1,22 @@
require 'spec_helper'
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
it 'returns Gitlab::Access.options_with_owner' do
expect(described_class.access_level_roles).to eq(Gitlab::Access.options_with_owner)
diff --git a/spec/services/projects/participants_service_spec.rb b/spec/services/projects/participants_service_spec.rb
index 6040f9100f83a..4b6d0c5136305 100644
--- a/spec/services/projects/participants_service_spec.rb
+++ b/spec/services/projects/participants_service_spec.rb
@@ -2,29 +2,56 @@
describe Projects::ParticipantsService 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
- let(:project) { create(:project, :public) }
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
- participants = described_class.new(project, user)
- groups = participants.groups
+ before do
+ group.add_owner(user)
+ end
- expect(groups.size).to eq 1
- expect(groups.first[:avatar_url]).to eq("/uploads/-/system/group/avatar/#{group.id}/dk.png")
+ it 'should return an url for the avatar' do
+ expect(service.groups.size).to eq 1
+ expect(service.groups.first[:avatar_url]).to eq("/uploads/-/system/group/avatar/#{group.id}/dk.png")
end
it 'should return an url for the avatar with relative url' do
stub_config_setting(relative_url_root: '/gitlab')
stub_config_setting(url: Settings.send(:build_gitlab_url))
- participants = described_class.new(project, user)
- groups = participants.groups
-
- expect(groups.size).to eq 1
- expect(groups.first[:avatar_url]).to eq("/gitlab/uploads/-/system/group/avatar/#{group.id}/dk.png")
+ expect(service.groups.size).to eq 1
+ expect(service.groups.first[:avatar_url]).to eq("/gitlab/uploads/-/system/group/avatar/#{group.id}/dk.png")
end
end
end
--
GitLab