From 7f73f440f9a4a8761c083a6781d3c4015228d9b9 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 31 May 2017 16:45:14 +0100 Subject: Fix N+1 queries for non-members in comment threads When getting the max member access for a group of users, we stored the results in RequestStore. However, this will only return results for project members, so anyone who wasn't a member of the project would be checked once at the start, and then once for each comment they made. These queries are generally quite fast, but no query is faster! --- spec/models/project_team_spec.rb | 151 +++++++++++++++++++++++++-------------- 1 file changed, 98 insertions(+), 53 deletions(-) (limited to 'spec') diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index fb2d5f60009..362565506e5 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -327,69 +327,114 @@ describe ProjectTeam, models: true do end end - shared_examples_for "#max_member_access_for_users" do |enable_request_store| - describe "#max_member_access_for_users" do + shared_examples 'max member access for users' do + let(:project) { create(:project) } + let(:group) { create(:group) } + let(:second_group) { create(:group) } + + let(:master) { create(:user) } + let(:reporter) { create(:user) } + let(:guest) { create(:user) } + + let(:promoted_guest) { create(:user) } + + let(:group_developer) { create(:user) } + let(:second_developer) { create(:user) } + + let(:user_without_access) { create(:user) } + let(:second_user_without_access) { create(:user) } + + let(:users) do + [master, reporter, promoted_guest, guest, group_developer, second_developer, user_without_access].map(&:id) + end + + let(:expected) do + { + master.id => Gitlab::Access::MASTER, + reporter.id => Gitlab::Access::REPORTER, + promoted_guest.id => Gitlab::Access::DEVELOPER, + guest.id => Gitlab::Access::GUEST, + group_developer.id => Gitlab::Access::DEVELOPER, + second_developer.id => Gitlab::Access::MASTER, + user_without_access.id => Gitlab::Access::NO_ACCESS + } + end + + before do + project.add_master(master) + project.add_reporter(reporter) + project.add_guest(promoted_guest) + project.add_guest(guest) + + project.project_group_links.create( + group: group, + group_access: Gitlab::Access::DEVELOPER + ) + + group.add_master(promoted_guest) + group.add_developer(group_developer) + group.add_developer(second_developer) + + project.project_group_links.create( + group: second_group, + group_access: Gitlab::Access::MASTER + ) + + second_group.add_master(second_developer) + end + + it 'returns correct roles for different users' do + expect(project.team.max_member_access_for_user_ids(users)).to eq(expected) + end + end + + describe '#max_member_access_for_user_ids' do + context 'with RequestStore enabled' do before do - RequestStore.begin! if enable_request_store + RequestStore.begin! end after do - if enable_request_store - RequestStore.end! - RequestStore.clear! - end + RequestStore.end! + RequestStore.clear! end - it 'returns correct roles for different users' do - master = create(:user) - reporter = create(:user) - promoted_guest = create(:user) - guest = create(:user) - project = create(:empty_project) + include_examples 'max member access for users' - project.add_master(master) - project.add_reporter(reporter) - project.add_guest(promoted_guest) - project.add_guest(guest) + def access_levels(users) + project.team.max_member_access_for_user_ids(users) + end + + it 'does not perform extra queries when asked for users who have already been found' do + access_levels(users) + + expect { access_levels(users) }.not_to exceed_query_limit(0) - group = create(:group) - group_developer = create(:user) - second_developer = create(:user) - project.project_group_links.create( - group: group, - group_access: Gitlab::Access::DEVELOPER) - - group.add_master(promoted_guest) - group.add_developer(group_developer) - group.add_developer(second_developer) - - second_group = create(:group) - project.project_group_links.create( - group: second_group, - group_access: Gitlab::Access::MASTER) - second_group.add_master(second_developer) - - users = [master, reporter, promoted_guest, guest, group_developer, second_developer].map(&:id) - - expected = { - master.id => Gitlab::Access::MASTER, - reporter.id => Gitlab::Access::REPORTER, - promoted_guest.id => Gitlab::Access::DEVELOPER, - guest.id => Gitlab::Access::GUEST, - group_developer.id => Gitlab::Access::DEVELOPER, - second_developer.id => Gitlab::Access::MASTER - } - - expect(project.team.max_member_access_for_user_ids(users)).to eq(expected) + expect(access_levels(users)).to eq(expected) end - end - end - describe '#max_member_access_for_users with RequestStore' do - it_behaves_like "#max_member_access_for_users", true - end + it 'only requests the extra users when uncached users are passed' do + new_user = create(:user) + second_new_user = create(:user) + all_users = users + [new_user.id, second_new_user.id] + + expected_all = expected.merge(new_user.id => Gitlab::Access::NO_ACCESS, + second_new_user.id => Gitlab::Access::NO_ACCESS) + + access_levels(users) - describe '#max_member_access_for_users without RequestStore' do - it_behaves_like "#max_member_access_for_users", false + queries = ActiveRecord::QueryRecorder.new { access_levels(all_users) } + + expect(queries.count).to eq(1) + expect(queries.log_message).to match(/\W#{new_user.id}\W/) + expect(queries.log_message).to match(/\W#{second_new_user.id}\W/) + expect(queries.log_message).not_to match(/\W#{promoted_guest.id}\W/) + expect(access_levels(all_users)).to eq(expected_all) + end + end + + context 'with RequestStore disabled' do + include_examples 'max member access for users' + end end end -- cgit v1.2.3