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

@@ -1,4 +1,5 @@
 class Projects::IssuesController < Projects::ApplicationController
+  include NotesHelper
   include ToggleSubscriptionAction
   include IssuableActions
   include ToggleAwardEmoji
@@ -70,6 +71,8 @@ def show
     @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 @@ def define_discussion_vars
       @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 @@ def note_target_fields(note)
   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 @@ def link_to_reply_discussion(discussion, line_type = nil)
     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
+  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
 
-    full_key = { project: note.project, user_id: note.author_id }
-    @max_access_by_user_id[full_key]
+  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 @@ def can_read_group?(user, group)
       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 @@ def human_max_access(user_id)
     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 = []
+  # 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]
 
-    access += project.members.where(user_id: user_id).has_access.pluck(:access_level)
+    # Lookup only the IDs we need
+    user_ids = user_ids - access.keys
 
-    if group
-      access += group.members.where(user_id: user_id).has_access.pluck(:access_level)
-    end
+    if user_ids.present?
+      user_ids.map { |id| access[id] = Gitlab::Access::NO_ACCESS }
 
-    if project.invited_groups.any? && project.allowed_to_share_with_group?
-      access << max_invited_level(user_id)
+      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 @@ def max_invited_level(user_id)
     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 @@ def fetch_members(level = nil)
   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

@@ -1,37 +1,30 @@
 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
+  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) }
 
-    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_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 @@
       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

@@ -1,6 +1,62 @@
 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 @@
         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 @@
 
         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 @@
       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