From a397a0eb1a4c34c27175e2c4e68e7ceb43a81f02 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 19 Jul 2017 19:12:11 +0800
Subject: [PATCH] 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.
---
 app/models/concerns/protected_ref.rb |  9 +++++----
 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
-- 
GitLab