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. --- 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 ++++++++++++++++------ 5 files changed, 62 insertions(+), 19 deletions(-) (limited to 'app') 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 -- cgit v1.2.3