From c5f9b2be826c05e5f06d424f5c110976ad0b68c4 Mon Sep 17 00:00:00 2001 From: Sean McGivern 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 2c9e1ba1d80..510f856087d 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 6713b6617ae..a3cc6014fd3 100644 --- a/app/services/concerns/users/participable_service.rb +++ b/app/services/concerns/users/participable_service.rb @@ -28,19 +28,35 @@ module 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 00000000000..ae097e859d9 --- /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 a3451c67bd8..bc937368cff 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 6040f9100f8..4b6d0c51363 100644 --- a/spec/services/projects/participants_service_spec.rb +++ b/spec/services/projects/participants_service_spec.rb @@ -2,29 +2,56 @@ require 'spec_helper' 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) } + + before do + group.add_owner(user) + end it 'should return an url for the avatar' do - participants = described_class.new(project, user) - groups = participants.groups - - expect(groups.size).to eq 1 - 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 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