Optimize maximum user access level lookup in loading of notes

NotesHelper#note_editable? and ProjectTeam#human_max_access currently
take about 16% of the load time of an issue page. This MR preloads
the maximum access level of users for all notes in issues and merge
requests with several queries instead of one per user and caches
the result in RequestStore.

CHANGELOG

@@ -5,6 +5,7 @@ v 8.11.0 (unreleased)
   - Fix CI status icon link underline (ClemMakesApps)
   - Fix of 'Commits being passed to custom hooks are already reachable when using the UI'
   - Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable
+  - Optimize maximum user access level lookup in loading of notes
   - Limit git rev-list output count to one in forced push check
   - Add green outline to New Branch button. !5447 (winniehell)
   - Retrieve rendered HTML from cache in one request

app/controllers/projects/issues_controller.rb

 class Projects::IssuesController < Projects::ApplicationController
+  include NotesHelper
   include ToggleSubscriptionAction
   include IssuableActions
   include ToggleAwardEmoji
@@ -70,6 +71,8 @@ class Projects::IssuesController < Projects::ApplicationController
     @note     = @project.notes.new(noteable: @issue)
     @noteable = @issue
+    preload_max_access_for_authors(@notes, @project) if @notes
+
     respond_to do |format|
       format.html
       format.json do

app/controllers/projects/merge_requests_controller.rb

@@ -3,6 +3,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   include DiffForPath
   include DiffHelper
   include IssuableActions
+  include NotesHelper
   include ToggleAwardEmoji
   before_action :module_enabled
@@ -385,6 +386,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController
       @project_wiki,
       @ref
     )
+
+    preload_max_access_for_authors(@notes, @project) if @notes
   end
   def define_widget_vars

app/helpers/notes_helper.rb

@@ -7,7 +7,7 @@ module NotesHelper
   end
   def note_editable?(note)
-    note.editable? && can?(current_user, :admin_note, note)
+    Ability.can_edit_note?(current_user, note)
   end
   def noteable_json(noteable)
@@ -87,14 +87,13 @@ module NotesHelper
     end
   end
-  def note_max_access_for_user(note)
-    @max_access_by_user_id ||= Hash.new do |hash, key|
-      project = key[:project]
-      hash[key] = project.team.human_max_access(key[:user_id])
-    end
-    full_key = { project: note.project, user_id: note.author_id }
-    @max_access_by_user_id[full_key]
+  def preload_max_access_for_authors(notes, project)
+    user_ids = notes.map(&:author_id)
+    project.team.max_member_access_for_user_ids(user_ids)
+  end
+  def note_max_access_for_user(note)
+    note.project.team.human_max_access(note.author_id)
   end
   def discussion_diff_path(discussion)

app/models/ability.rb

@@ -388,6 +388,20 @@ class Ability
       GroupProjectsFinder.new(group).execute(user).any?
     end
+    def can_edit_note?(user, note)
+      return false unless note.editable?
+      return false unless user.present?
+      return true if note.author == user
+      return true if user.admin?
+
+      if note.project
+        max_access_level = note.project.team.max_member_access(user.id)
+        max_access_level >= Gitlab::Access::MASTER
+      else
+        false
+      end
+    end
+
     def namespace_abilities(user, namespace)
       rules = []

app/models/project_team.rb

@@ -132,22 +132,41 @@ class ProjectTeam
     Gitlab::Access.options_with_owner.key(max_member_access(user_id))
   end
-  # This method assumes project and group members are eager loaded for optimal
-  # performance.
-  def max_member_access(user_id)
-    access = []
-    access += project.members.where(user_id: user_id).has_access.pluck(:access_level)
-    if group
-      access += group.members.where(user_id: user_id).has_access.pluck(:access_level)
-    end
-    if project.invited_groups.any? && project.allowed_to_share_with_group?
-      access << max_invited_level(user_id)
+  # Determine the maximum access level for a group of users in bulk.
+  #
+  # Returns a Hash mapping user ID -> maxmum access level.
+  def max_member_access_for_user_ids(user_ids)
+    user_ids = user_ids.uniq
+    key = "max_member_access:#{project.id}"
+    RequestStore.store[key] ||= Hash.new
+    access = RequestStore.store[key]
+    # Lookup only the IDs we need
+    user_ids = user_ids - access.keys
+    if user_ids.present?
+      user_ids.map { |id| access[id] = Gitlab::Access::NO_ACCESS }
+      member_access = project.members.where(user_id: user_ids).has_access.pluck(:user_id, :access_level).to_h
+      merge_max!(access, member_access)
+
+      if group
+        group_access = group.members.where(user_id: user_ids).has_access.pluck(:user_id, :access_level).to_h
+        merge_max!(access, group_access)
+      end
+
+      if project.invited_groups.any? && project.allowed_to_share_with_group?
+        # Not optimized
+        invited_levels = user_ids.map { |id| [id, max_invited_level(id)] }.to_h
+        merge_max!(access, invited_levels)
+      end
     end
-    access.compact.max
+    access
+  end
+
+  def max_member_access(user_id)
+    max_member_access_for_user_ids([user_id])[user_id]
   end
   private
@@ -156,6 +175,7 @@ class ProjectTeam
     project.project_group_links.map do |group_link|
       invited_group = group_link.group
       access = invited_group.group_members.find_by(user_id: user_id).try(:access_field)
+      access = Gitlab::Access::NO_ACCESS unless access.present?
       # If group member has higher access level we should restrict it
       # to max allowed access level
@@ -215,4 +235,8 @@ class ProjectTeam
   def group
     project.group
   end
+
+  def merge_max!(first_hash, second_hash)
+    first_hash.merge!(second_hash) { |_key, old, new| old > new ? old : new }
+  end
 end

lib/gitlab/access.rb

@@ -7,6 +7,7 @@ module Gitlab
   module Access
     class AccessDeniedError < StandardError; end
+    NO_ACCESS = 0
     GUEST     = 10
     REPORTER  = 20
     DEVELOPER = 30

spec/helpers/notes_helper_spec.rb

 require "spec_helper"
 describe NotesHelper do
-  describe "#notes_max_access_for_users" do
-    let(:owner) { create(:owner) }
-    let(:group) { create(:group) }
-    let(:project) { create(:empty_project, namespace: group) }
-    let(:master) { create(:user) }
-    let(:reporter) { create(:user) }
-    let(:guest) { create(:user) }
-
-    let(:owner_note) { create(:note, author: owner, project: project) }
-    let(:master_note) { create(:note, author: master, project: project) }
-    let(:reporter_note) { create(:note, author: reporter, project: project) }
-    let!(:notes) { [owner_note, master_note, reporter_note] }
-
-    before do
-      group.add_owner(owner)
-      project.team << [master, :master]
-      project.team << [reporter, :reporter]
-      project.team << [guest, :guest]
-    end
-    it 'return human access levels' do
-      original_method = project.team.method(:human_max_access)
-      expect_any_instance_of(ProjectTeam).to receive(:human_max_access).exactly(3).times do |*args|
-        original_method.call(args[1])
-      end
+  let(:owner) { create(:owner) }
+  let(:group) { create(:group) }
+  let(:project) { create(:empty_project, namespace: group) }
+  let(:master) { create(:user) }
+  let(:reporter) { create(:user) }
+  let(:guest) { create(:user) }
+  let(:owner_note) { create(:note, author: owner, project: project) }
+  let(:master_note) { create(:note, author: master, project: project) }
+  let(:reporter_note) { create(:note, author: reporter, project: project) }
+  let!(:notes) { [owner_note, master_note, reporter_note] }
+  before do
+    group.add_owner(owner)
+    project.team << [master, :master]
+    project.team << [reporter, :reporter]
+    project.team << [guest, :guest]
+  end
+
+  describe "#notes_max_access_for_users" do
+    it 'return human access levels' do
       expect(helper.note_max_access_for_user(owner_note)).to eq('Owner')
       expect(helper.note_max_access_for_user(master_note)).to eq('Master')
       expect(helper.note_max_access_for_user(reporter_note)).to eq('Reporter')
-      # Call it again to ensure value is cached
-      expect(helper.note_max_access_for_user(owner_note)).to eq('Owner')
     end
     it 'handles access in different projects' do
@@ -43,4 +36,16 @@ describe NotesHelper do
       expect(helper.note_max_access_for_user(other_note)).to eq('Reporter')
     end
   end
+
+  describe '#preload_max_access_for_authors' do
+    it 'loads multiple users' do
+      expected_access = {
+        owner.id => Gitlab::Access::OWNER,
+        master.id => Gitlab::Access::MASTER,
+        reporter.id => Gitlab::Access::REPORTER
+      }
+
+      expect(helper.preload_max_access_for_authors(notes, project)).to eq(expected_access)
+    end
+  end
 end

spec/models/ability_spec.rb

 require 'spec_helper'
 describe Ability, lib: true do
+  describe '.can_edit_note?' do
+    let(:project) { create(:empty_project) }
+    let!(:note) { create(:note_on_issue, project: project) }
+
+    context 'using an anonymous user' do
+      it 'returns false' do
+        expect(described_class.can_edit_note?(nil, note)).to be_falsy
+      end
+    end
+
+    context 'using a system note' do
+      it 'returns false' do
+        system_note = create(:note, system: true)
+        user = create(:user)
+
+        expect(described_class.can_edit_note?(user, system_note)).to be_falsy
+      end
+    end
+
+    context 'using users with different access levels' do
+      let(:user) { create(:user) }
+
+      it 'returns true for the author' do
+        expect(described_class.can_edit_note?(note.author, note)).to be_truthy
+      end
+
+      it 'returns false for a guest user' do
+        project.team << [user, :guest]
+
+        expect(described_class.can_edit_note?(user, note)).to be_falsy
+      end
+
+      it 'returns false for a developer' do
+        project.team << [user, :developer]
+
+        expect(described_class.can_edit_note?(user, note)).to be_falsy
+      end
+
+      it 'returns true for a master' do
+        project.team << [user, :master]
+
+        expect(described_class.can_edit_note?(user, note)).to be_truthy
+      end
+
+      it 'returns true for a group owner' do
+        group = create(:group)
+        project.project_group_links.create(
+          group: group,
+          group_access: Gitlab::Access::MASTER)
+        group.add_owner(user)
+
+        expect(described_class.can_edit_note?(user, note)).to be_truthy
+      end
+    end
+  end
+
   describe '.users_that_can_read_project' do
     context 'using a public project' do
       it 'returns all the users' do

spec/models/project_team_spec.rb

@@ -151,8 +151,8 @@ describe ProjectTeam, models: true do
         it { expect(project.team.max_member_access(master.id)).to eq(Gitlab::Access::MASTER) }
         it { expect(project.team.max_member_access(reporter.id)).to eq(Gitlab::Access::REPORTER) }
         it { expect(project.team.max_member_access(guest.id)).to eq(Gitlab::Access::GUEST) }
-        it { expect(project.team.max_member_access(nonmember.id)).to be_nil }
-        it { expect(project.team.max_member_access(requester.id)).to be_nil }
+        it { expect(project.team.max_member_access(nonmember.id)).to eq(Gitlab::Access::NO_ACCESS) }
+        it { expect(project.team.max_member_access(requester.id)).to eq(Gitlab::Access::NO_ACCESS) }
       end
       context 'when project is shared with group' do
@@ -168,14 +168,14 @@ describe ProjectTeam, models: true do
         it { expect(project.team.max_member_access(master.id)).to eq(Gitlab::Access::DEVELOPER) }
         it { expect(project.team.max_member_access(reporter.id)).to eq(Gitlab::Access::REPORTER) }
-        it { expect(project.team.max_member_access(nonmember.id)).to be_nil }
-        it { expect(project.team.max_member_access(requester.id)).to be_nil }
+        it { expect(project.team.max_member_access(nonmember.id)).to eq(Gitlab::Access::NO_ACCESS) }
+        it { expect(project.team.max_member_access(requester.id)).to eq(Gitlab::Access::NO_ACCESS) }
         context 'but share_with_group_lock is true' do
           before { project.namespace.update(share_with_group_lock: true) }
-          it { expect(project.team.max_member_access(master.id)).to be_nil }
-          it { expect(project.team.max_member_access(reporter.id)).to be_nil }
+          it { expect(project.team.max_member_access(master.id)).to eq(Gitlab::Access::NO_ACCESS) }
+          it { expect(project.team.max_member_access(reporter.id)).to eq(Gitlab::Access::NO_ACCESS) }
         end
       end
     end
@@ -194,8 +194,43 @@ describe ProjectTeam, models: true do
       it { expect(project.team.max_member_access(master.id)).to eq(Gitlab::Access::MASTER) }
       it { expect(project.team.max_member_access(reporter.id)).to eq(Gitlab::Access::REPORTER) }
       it { expect(project.team.max_member_access(guest.id)).to eq(Gitlab::Access::GUEST) }
-      it { expect(project.team.max_member_access(nonmember.id)).to be_nil }
-      it { expect(project.team.max_member_access(requester.id)).to be_nil }
+      it { expect(project.team.max_member_access(nonmember.id)).to eq(Gitlab::Access::NO_ACCESS) }
+      it { expect(project.team.max_member_access(requester.id)).to eq(Gitlab::Access::NO_ACCESS) }
+    end
+  end
+
+  describe "#max_member_access_for_users" do
+    it 'returns correct roles for different users' do
+      master = create(:user)
+      reporter = create(:user)
+      promoted_guest = create(:user)
+      guest = create(:user)
+      project = create(:project)
+
+      project.team << [master, :master]
+      project.team << [reporter, :reporter]
+      project.team << [promoted_guest, :guest]
+      project.team << [guest, :guest]
+
+      group = create(:group)
+      group_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)
+      users = [master, reporter, promoted_guest, guest, group_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
+      }
+
+      expect(project.team.max_member_access_for_user_ids(users)).to eq(expected)
     end
   end
 end