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!

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
 

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:

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
-      before do
-        RequestStore.begin! if enable_request_store
-      end
+  shared_examples 'max member access for users' do
+    let(:project) { create(:project) }
+    let(:group) { create(:group) }
+    let(:second_group) { create(:group) }
 
-      after do
-        if enable_request_store
-          RequestStore.end!
-          RequestStore.clear!
-        end
-      end
+    let(:master) { create(:user) }
+    let(:reporter) { create(:user) }
+    let(:guest) { create(:user) }
 
-      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)
+    let(:promoted_guest) { create(:user) }
 
-        project.add_master(master)
-        project.add_reporter(reporter)
-        project.add_guest(promoted_guest)
-        project.add_guest(guest)
+    let(:group_developer) { create(:user) }
+    let(:second_developer) { create(:user) }
 
-        group = create(:group)
-        group_developer = create(:user)
-        second_developer = create(:user)
-        project.project_group_links.create(
-          group: group,
-          group_access: Gitlab::Access::DEVELOPER)
+    let(:user_without_access) { create(:user) }
+    let(:second_user_without_access) { create(:user) }
 
-        group.add_master(promoted_guest)
-        group.add_developer(group_developer)
-        group.add_developer(second_developer)
+    let(:users) do
+      [master, reporter, promoted_guest, guest, group_developer, second_developer, user_without_access].map(&:id)
+    end
 
-        second_group = create(:group)
-        project.project_group_links.create(
-          group: second_group,
-          group_access: Gitlab::Access::MASTER)
-        second_group.add_master(second_developer)
+    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
 
-        users = [master, reporter, promoted_guest, guest, group_developer, second_developer].map(&:id)
+    before do
+      project.add_master(master)
+      project.add_reporter(reporter)
+      project.add_guest(promoted_guest)
+      project.add_guest(guest)
 
-        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
-        }
+      project.project_group_links.create(
+        group: group,
+        group_access: Gitlab::Access::DEVELOPER
+      )
 
-        expect(project.team.max_member_access_for_user_ids(users)).to eq(expected)
-      end
+      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_users with RequestStore' do
-    it_behaves_like "#max_member_access_for_users", true
-  end
+  describe '#max_member_access_for_user_ids' do
+    context 'with RequestStore enabled' do
+      before do
+        RequestStore.begin!
+      end
 
-  describe '#max_member_access_for_users without RequestStore' do
-    it_behaves_like "#max_member_access_for_users", false
+      after do
+        RequestStore.end!
+        RequestStore.clear!
+      end
+
+      include_examples 'max member access for users'
+
+      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)
+
+        expect(access_levels(users)).to eq(expected)
+      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)
+
+        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