summaryrefslogtreecommitdiff
path: root/lib
diff options
context:
space:
mode:
authorLin Jen-Shin <godfat@godfat.org>2017-07-17 23:24:46 +0800
committerLin Jen-Shin <godfat@godfat.org>2017-07-17 23:24:46 +0800
commit143fc48abac6e278dcda9be4b90cb3ca1291f4d9 (patch)
tree4792cf5eff9665810c52d27507f64ea17465527a /lib
parent05329d4a364a5c55f2de9546871de1909b6be3f5 (diff)
downloadgitlab-ce-143fc48abac6e278dcda9be4b90cb3ca1291f4d9.tar.gz
Add RequestStoreWrap to cache via RequestStore
I don't like the idea of `RequestStore` at all, because it's just a global state which shouldn't be used at all. But we have a number of places calling `ProtectedBranch.protected?` and `ProtectedTag.protected?` in a loop for the same user, project, and ref whenever we're checking against if the jobs for a given pipeline is accessible for a given user. This means we're effectively making N queries for the same thing over and over. To properly fix this, we need to change how we check the permission, and that could be a huge work. To solve this quickly, adding a cache layer for the given request would be quite simple to do. We're already doing this in Commit#author, and this is extending that idea and make it generalized.
Diffstat (limited to 'lib')
-rw-r--r--lib/gitlab/cache/request_store_wrap.rb60
-rw-r--r--lib/gitlab/user_access.rb14
2 files changed, 70 insertions, 4 deletions
diff --git a/lib/gitlab/cache/request_store_wrap.rb b/lib/gitlab/cache/request_store_wrap.rb
new file mode 100644
index 00000000000..3e0a5f06b53
--- /dev/null
+++ b/lib/gitlab/cache/request_store_wrap.rb
@@ -0,0 +1,60 @@
+module Gitlab
+ module Cache
+ # This module provides a simple way to cache values in RequestStore,
+ # and the cache key would be based on the class name, method name,
+ # customized instance level values, and arguments.
+ #
+ # A simple example:
+ #
+ # class UserAccess
+ # extend Gitlab::Cache::RequestStoreWrap
+ #
+ # request_store_wrap_key do
+ # [user.id, project.id]
+ # end
+ #
+ # request_store_wrap def can_push_to_branch?(ref)
+ # # ...
+ # end
+ # end
+ #
+ # This way, the result of `can_push_to_branch?` would be cached in
+ # `RequestStore.store` based on the cache key.
+ module RequestStoreWrap
+ def self.extended(klass)
+ return if klass < self
+
+ extension = Module.new
+ klass.const_set(:RequestStoreWrapExtension, extension)
+ klass.prepend(extension)
+ end
+
+ def request_store_wrap_key(&block)
+ if block_given?
+ @request_store_wrap_key = block
+ else
+ @request_store_wrap_key
+ end
+ end
+
+ def request_store_wrap(method_name)
+ const_get(:RequestStoreWrapExtension)
+ .send(:define_method, method_name) do |*args|
+ return super(*args) unless RequestStore.active?
+
+ klass = self.class
+ key = [klass.name,
+ method_name,
+ *instance_exec(&klass.request_store_wrap_key),
+ *args].join(':')
+
+ if RequestStore.store.key?(key)
+ RequestStore.store[key]
+ else
+ RequestStore.store[key] = super(*args)
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index 3b922da7ced..9c066627011 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -1,5 +1,11 @@
module Gitlab
class UserAccess
+ extend Gitlab::Cache::RequestStoreWrap
+
+ request_store_wrap_key do
+ [user&.id, project&.id]
+ end
+
attr_reader :user, :project
def initialize(user, project: nil)
@@ -28,7 +34,7 @@ module Gitlab
true
end
- def can_create_tag?(ref)
+ request_store_wrap def can_create_tag?(ref)
return false unless can_access_git?
if ProtectedTag.protected?(project, ref)
@@ -38,7 +44,7 @@ module Gitlab
end
end
- def can_delete_branch?(ref)
+ request_store_wrap def can_delete_branch?(ref)
return false unless can_access_git?
if ProtectedBranch.protected?(project, ref)
@@ -48,7 +54,7 @@ module Gitlab
end
end
- def can_push_to_branch?(ref)
+ request_store_wrap def can_push_to_branch?(ref)
return false unless can_access_git?
if ProtectedBranch.protected?(project, ref)
@@ -60,7 +66,7 @@ module Gitlab
end
end
- def can_merge_to_branch?(ref)
+ request_store_wrap def can_merge_to_branch?(ref)
return false unless can_access_git?
if ProtectedBranch.protected?(project, ref)