Commit a397a0eb authored by Lin Jen-Shin's avatar Lin Jen-Shin

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.
parent 561bc570
...@@ -25,8 +25,8 @@ module ProtectedRef ...@@ -25,8 +25,8 @@ module ProtectedRef
end end
end end
def protected_ref_accessible_to?(ref, user, action:) def protected_ref_accessible_to?(ref, user, action:, protected_refs: nil)
access_levels_for_ref(ref, action: action).any? do |access_level| access_levels_for_ref(ref, action: action, protected_refs: protected_refs).any? do |access_level|
access_level.check_access(user) access_level.check_access(user)
end end
end end
...@@ -37,8 +37,9 @@ module ProtectedRef ...@@ -37,8 +37,9 @@ module ProtectedRef
end end
end end
def access_levels_for_ref(ref, action:) def access_levels_for_ref(ref, action:, protected_refs: nil)
self.matching(ref).map(&:"#{action}_access_levels").flatten self.matching(ref, protected_refs: protected_refs)
.map(&:"#{action}_access_levels").flatten
end end
def matching(ref_name, protected_refs: nil) def matching(ref_name, protected_refs: nil)
......
...@@ -37,8 +37,8 @@ module Gitlab ...@@ -37,8 +37,8 @@ module Gitlab
request_cache def can_create_tag?(ref) request_cache def can_create_tag?(ref)
return false unless can_access_git? return false unless can_access_git?
if ProtectedTag.protected?(project, ref) if protected?(ProtectedTag, project, ref)
project.protected_tags.protected_ref_accessible_to?(ref, user, action: :create) protected_tag_accessible_to?(ref, action: :create)
else else
user.can?(:push_code, project) user.can?(:push_code, project)
end end
...@@ -47,7 +47,7 @@ module Gitlab ...@@ -47,7 +47,7 @@ module Gitlab
request_cache def can_delete_branch?(ref) request_cache def can_delete_branch?(ref)
return false unless can_access_git? return false unless can_access_git?
if ProtectedBranch.protected?(project, ref) if protected?(ProtectedBranch, project, ref)
user.can?(:delete_protected_branch, project) user.can?(:delete_protected_branch, project)
else else
user.can?(:push_code, project) user.can?(:push_code, project)
...@@ -61,10 +61,10 @@ module Gitlab ...@@ -61,10 +61,10 @@ module Gitlab
request_cache def can_push_to_branch?(ref) request_cache def can_push_to_branch?(ref)
return false unless can_access_git? 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) 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 else
user.can?(:push_code, project) user.can?(:push_code, project)
end end
...@@ -73,8 +73,8 @@ module Gitlab ...@@ -73,8 +73,8 @@ module Gitlab
request_cache def can_merge_to_branch?(ref) request_cache def can_merge_to_branch?(ref)
return false unless can_access_git? return false unless can_access_git?
if ProtectedBranch.protected?(project, ref) if protected?(ProtectedBranch, project, ref)
project.protected_branches.protected_ref_accessible_to?(ref, user, action: :merge) protected_branch_accessible_to?(ref, action: :merge)
else else
user.can?(:push_code, project) user.can?(:push_code, project)
end end
...@@ -91,5 +91,21 @@ module Gitlab ...@@ -91,5 +91,21 @@ module Gitlab
def can_access_git? def can_access_git?
user && user.can?(:access_git) user && user.can?(:access_git)
end 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
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment