From d1ea2bca61dff21948024d897e1d4475123a10e8 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 19 Jul 2016 21:52:31 -0700 Subject: 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 | 1 + app/controllers/projects/issues_controller.rb | 3 ++ .../projects/merge_requests_controller.rb | 3 ++ app/helpers/notes_helper.rb | 15 +++--- app/models/ability.rb | 14 ++++++ app/models/project_team.rb | 46 ++++++++++++----- lib/gitlab/access.rb | 1 + spec/helpers/notes_helper_spec.rb | 57 ++++++++++++---------- spec/models/ability_spec.rb | 56 +++++++++++++++++++++ spec/models/project_team_spec.rb | 51 ++++++++++++++++--- 10 files changed, 194 insertions(+), 53 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 5ff0cb42ccc..fb38db15630 100644 --- a/CHANGELOG +++ b/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 diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index fa663c9bda4..16ed7c2b6b4 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/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 @@ 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 diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 594a61464b9..da1b9c3e48a 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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 diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 0f60dd828ab..0c47abe0fba 100644 --- a/app/helpers/notes_helper.rb +++ b/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 + 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) diff --git a/app/models/ability.rb b/app/models/ability.rb index f33c8d61d3f..6884d99c5a6 100644 --- a/app/models/ability.rb +++ b/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 = [] diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 9d312a53790..67faea1f9f3 100644 --- a/app/models/project_team.rb +++ b/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 = [] + # 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 @@ 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 diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index de41ea415a6..a533bac2692 100644 --- a/lib/gitlab/access.rb +++ b/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 diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb index 08a93503258..af371248ae9 100644 --- a/spec/helpers/notes_helper_spec.rb +++ b/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 @@ 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 diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 1acb5846fcf..cd5f40fe3d2 100644 --- a/spec/models/ability_spec.rb +++ b/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 diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index 9262aeb6ed8..115fffd82d9 100644 --- a/spec/models/project_team_spec.rb +++ b/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 -- cgit v1.2.3