diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2017-07-19 14:12:11 +0300 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2017-07-19 14:12:11 +0300 |
commit | a397a0eb1a4c34c27175e2c4e68e7ceb43a81f02 (patch) | |
tree | ea480b619d43172143e8a7c07859da3ba69efa86 /lib/gitlab/user_access.rb | |
parent | 561bc570dea970328e0c33972fcf1ed90427f2f2 (diff) |
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.
Diffstat (limited to 'lib/gitlab/user_access.rb')
-rw-r--r-- | lib/gitlab/user_access.rb | 30 |
1 files changed, 23 insertions, 7 deletions
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 |