diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2017-07-19 19:12:11 +0800 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2017-07-19 19:12:11 +0800 |
commit | a397a0eb1a4c34c27175e2c4e68e7ceb43a81f02 (patch) | |
tree | ea480b619d43172143e8a7c07859da3ba69efa86 | |
parent | 561bc570dea970328e0c33972fcf1ed90427f2f2 (diff) | |
download | gitlab-ce-a397a0eb1a4c34c27175e2c4e68e7ceb43a81f02.tar.gz |
Eliminate N+1 queries on checking different protected refs
I realized where the N+1 queries were actually coming from
project.protected_branches, but how come we cannot preload this,
or cache this at all?
Then I found that this is somehow a Rails limitation. What we're
doing before, eventually come to:
project.protected_branches.matching
But why it's not cached? (project.protected_branches.loaded? is always
false) It's because matching is a class method, which is called on
the proxy. In this case, Rails cannot cache the result. I don't know
if this is possible to implement or not, because clearly this would
require some tricks to implement class methods on associations.
So instead, we could just pass project.protected_branches to
ProtectedRef.matching, then it would work regularly.
With this change, there's no more N+1 queries.
-rw-r--r-- | app/models/concerns/protected_ref.rb | 9 | ||||
-rw-r--r-- | lib/gitlab/user_access.rb | 30 |
2 files changed, 28 insertions, 11 deletions
diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index fc6b840f7a8..ca9ef2b9375 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -25,8 +25,8 @@ module ProtectedRef end end - def protected_ref_accessible_to?(ref, user, action:) - access_levels_for_ref(ref, action: action).any? do |access_level| + def protected_ref_accessible_to?(ref, user, action:, protected_refs: nil) + access_levels_for_ref(ref, action: action, protected_refs: protected_refs).any? do |access_level| access_level.check_access(user) end end @@ -37,8 +37,9 @@ module ProtectedRef end end - def access_levels_for_ref(ref, action:) - self.matching(ref).map(&:"#{action}_access_levels").flatten + def access_levels_for_ref(ref, action:, protected_refs: nil) + self.matching(ref, protected_refs: protected_refs) + .map(&:"#{action}_access_levels").flatten end def matching(ref_name, protected_refs: nil) diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 25698bb8e99..6c6111006b6 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -37,8 +37,8 @@ module Gitlab request_cache def can_create_tag?(ref) return false unless can_access_git? - if ProtectedTag.protected?(project, ref) - project.protected_tags.protected_ref_accessible_to?(ref, user, action: :create) + if protected?(ProtectedTag, project, ref) + protected_tag_accessible_to?(ref, action: :create) else user.can?(:push_code, project) end @@ -47,7 +47,7 @@ module Gitlab request_cache def can_delete_branch?(ref) return false unless can_access_git? - if ProtectedBranch.protected?(project, ref) + if protected?(ProtectedBranch, project, ref) user.can?(:delete_protected_branch, project) else user.can?(:push_code, project) @@ -61,10 +61,10 @@ module Gitlab request_cache def can_push_to_branch?(ref) return false unless can_access_git? - if ProtectedBranch.protected?(project, ref) + if protected?(ProtectedBranch, project, ref) return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user) - project.protected_branches.protected_ref_accessible_to?(ref, user, action: :push) + protected_branch_accessible_to?(ref, action: :push) else user.can?(:push_code, project) end @@ -73,8 +73,8 @@ module Gitlab request_cache def can_merge_to_branch?(ref) return false unless can_access_git? - if ProtectedBranch.protected?(project, ref) - project.protected_branches.protected_ref_accessible_to?(ref, user, action: :merge) + if protected?(ProtectedBranch, project, ref) + protected_branch_accessible_to?(ref, action: :merge) else user.can?(:push_code, project) end @@ -91,5 +91,21 @@ module Gitlab def can_access_git? user && user.can?(:access_git) end + + def protected_branch_accessible_to?(ref, action:) + ProtectedBranch.protected_ref_accessible_to?( + ref, user, action: action, + protected_refs: project.protected_branches) + end + + def protected_tag_accessible_to?(ref, action:) + ProtectedTag.protected_ref_accessible_to?( + ref, user, action: action, + protected_refs: project.protected_tags) + end + + request_cache def protected?(kind, project, ref) + kind.protected?(project, ref) + end end end |