summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@gitlab.com>2017-05-31 16:45:14 +0100
committerSean McGivern <sean@gitlab.com>2017-06-01 10:21:26 +0100
commit7f73f440f9a4a8761c083a6781d3c4015228d9b9 (patch)
tree8119689770dfa79aaf1e77fc7c6d06a7b8b6cca2
parent228926daee799c95e752a3c284c860e5bc60e528 (diff)
downloadgitlab-ce-7f73f440f9a4a8761c083a6781d3c4015228d9b9.tar.gz
Fix N+1 queries for non-members in comment threadsfix-n-plus-one-queries-for-user-access
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!
-rw-r--r--app/models/project_team.rb9
-rw-r--r--changelogs/unreleased/fix-n-plus-one-queries-for-user-access.yml4
-rw-r--r--spec/models/project_team_spec.rb151
3 files changed, 110 insertions, 54 deletions
diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index 543b9b293e0..e1cc56551ba 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -167,7 +167,7 @@ class ProjectTeam
access = RequestStore.store[key]
end
- # Lookup only the IDs we need
+ # Look up only the IDs we need
user_ids = user_ids - access.keys
return access if user_ids.empty?
@@ -178,6 +178,13 @@ class ProjectTeam
maximum(:access_level)
access.merge!(users_access)
+
+ missing_user_ids = user_ids - users_access.keys
+
+ missing_user_ids.each do |user_id|
+ access[user_id] = Gitlab::Access::NO_ACCESS
+ end
+
access
end
diff --git a/changelogs/unreleased/fix-n-plus-one-queries-for-user-access.yml b/changelogs/unreleased/fix-n-plus-one-queries-for-user-access.yml
new file mode 100644
index 00000000000..c2671a96b83
--- /dev/null
+++ b/changelogs/unreleased/fix-n-plus-one-queries-for-user-access.yml
@@ -0,0 +1,4 @@
+---
+title: Fix N+1 queries for non-members in comment threads
+merge_request:
+author:
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