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!

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