Use `Gitlab::SafeRequestStore` in more places
Even if it doesn’t save lines of code, since people will tend to use code they’ve seen. And `SafeRequestStore` is safer since you don’t have to remember to check `RequestStore.active?`.
This commit is contained in:
parent
22bf3848ef
commit
a54a5d9f39
|
@ -74,7 +74,7 @@ class Ability
|
|||
end
|
||||
|
||||
def policy_for(user, subject = :global)
|
||||
cache = RequestStore.active? ? RequestStore : {}
|
||||
cache = Gitlab::SafeRequestStore.active? ? Gitlab::SafeRequestStore : {}
|
||||
DeclarativePolicy.policy_for(user, subject, cache: cache)
|
||||
end
|
||||
|
||||
|
|
|
@ -16,9 +16,9 @@ module BulkMemberAccessLoad
|
|||
key = max_member_access_for_resource_key(resource_klass, memoization_index)
|
||||
access = {}
|
||||
|
||||
if RequestStore.active?
|
||||
RequestStore.store[key] ||= {}
|
||||
access = RequestStore.store[key]
|
||||
if Gitlab::SafeRequestStore.active?
|
||||
Gitlab::SafeRequestStore[key] ||= {}
|
||||
access = Gitlab::SafeRequestStore[key]
|
||||
end
|
||||
|
||||
# Look up only the IDs we need
|
||||
|
|
|
@ -148,8 +148,8 @@ class Namespace < ActiveRecord::Base
|
|||
def find_fork_of(project)
|
||||
return nil unless project.fork_network
|
||||
|
||||
if RequestStore.active?
|
||||
forks_in_namespace = RequestStore.fetch("namespaces:#{id}:forked_projects") do
|
||||
if Gitlab::SafeRequestStore.active?
|
||||
forks_in_namespace = Gitlab::SafeRequestStore.fetch("namespaces:#{id}:forked_projects") do
|
||||
Hash.new do |found_forks, project|
|
||||
found_forks[project] = project.fork_network.find_forks_in(projects).first
|
||||
end
|
||||
|
|
|
@ -296,7 +296,7 @@ module Banzai
|
|||
|
||||
# Returns projects for the given paths.
|
||||
def find_for_paths(paths)
|
||||
if RequestStore.active?
|
||||
if Gitlab::SafeRequestStore.active?
|
||||
cache = refs_cache
|
||||
to_query = paths - cache.keys
|
||||
|
||||
|
@ -340,7 +340,7 @@ module Banzai
|
|||
end
|
||||
|
||||
def refs_cache
|
||||
RequestStore["banzai_#{parent_type}_refs".to_sym] ||= {}
|
||||
Gitlab::SafeRequestStore["banzai_#{parent_type}_refs".to_sym] ||= {}
|
||||
end
|
||||
|
||||
def parent_type
|
||||
|
|
|
@ -166,7 +166,7 @@ module Banzai
|
|||
# objects that have not yet been queried. For objects that have already
|
||||
# been queried the object is returned from the cache.
|
||||
def collection_objects_for_ids(collection, ids)
|
||||
if RequestStore.active?
|
||||
if Gitlab::SafeRequestStore.active?
|
||||
ids = ids.map(&:to_i)
|
||||
cache = collection_cache[collection_cache_key(collection)]
|
||||
to_query = ids - cache.keys
|
||||
|
@ -248,7 +248,7 @@ module Banzai
|
|||
end
|
||||
|
||||
def collection_cache
|
||||
RequestStore[:banzai_collection_cache] ||= Hash.new do |hash, key|
|
||||
Gitlab::SafeRequestStore[:banzai_collection_cache] ||= Hash.new do |hash, key|
|
||||
hash[key] = {}
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1,8 +1,8 @@
|
|||
module Banzai
|
||||
module RequestStoreReferenceCache
|
||||
def cached_call(request_store_key, cache_key, path: [])
|
||||
if RequestStore.active?
|
||||
cache = RequestStore[request_store_key] ||= Hash.new do |hash, key|
|
||||
if Gitlab::SafeRequestStore.active?
|
||||
cache = Gitlab::SafeRequestStore[request_store_key] ||= Hash.new do |hash, key|
|
||||
hash[key] = Hash.new { |h, k| h[k] = {} }
|
||||
end
|
||||
|
||||
|
|
|
@ -26,8 +26,8 @@ module Gitlab
|
|||
|
||||
define_method(method_name) do |*args|
|
||||
store =
|
||||
if RequestStore.active?
|
||||
RequestStore.store
|
||||
if Gitlab::SafeRequestStore.active?
|
||||
Gitlab::SafeRequestStore.store
|
||||
else
|
||||
ivar_name = # ! and ? cannot be used as ivar name
|
||||
"@cache_#{method_name.to_s.tr('!?', "\u2605\u2606")}"
|
||||
|
|
|
@ -47,7 +47,7 @@ module Gitlab
|
|||
end
|
||||
|
||||
def appearance
|
||||
RequestStore.store[:appearance] ||= (Appearance.current || Appearance.new)
|
||||
Gitlab::SafeRequestStore[:appearance] ||= (Appearance.current || Appearance.new)
|
||||
end
|
||||
|
||||
def appearance_favicon
|
||||
|
|
|
@ -17,18 +17,18 @@ module Gitlab
|
|||
].freeze
|
||||
|
||||
def self.set(gl_repository, env)
|
||||
return unless RequestStore.active?
|
||||
return unless Gitlab::SafeRequestStore.active?
|
||||
|
||||
raise "missing gl_repository" if gl_repository.blank?
|
||||
|
||||
RequestStore.store[:gitlab_git_env] ||= {}
|
||||
RequestStore.store[:gitlab_git_env][gl_repository] = whitelist_git_env(env)
|
||||
Gitlab::SafeRequestStore[:gitlab_git_env] ||= {}
|
||||
Gitlab::SafeRequestStore[:gitlab_git_env][gl_repository] = whitelist_git_env(env)
|
||||
end
|
||||
|
||||
def self.all(gl_repository)
|
||||
return {} unless RequestStore.active?
|
||||
return {} unless Gitlab::SafeRequestStore.active?
|
||||
|
||||
h = RequestStore.fetch(:gitlab_git_env) { {} }
|
||||
h = Gitlab::SafeRequestStore.fetch(:gitlab_git_env) { {} }
|
||||
h.fetch(gl_repository, {})
|
||||
end
|
||||
|
||||
|
|
|
@ -11,7 +11,7 @@ module Gitlab
|
|||
to: :failure_info
|
||||
|
||||
def self.for_storage(storage)
|
||||
cached_circuitbreakers = RequestStore.fetch(:circuitbreaker_cache) do
|
||||
cached_circuitbreakers = Gitlab::SafeRequestStore.fetch(:circuitbreaker_cache) do
|
||||
Hash.new do |hash, storage_name|
|
||||
hash[storage_name] = build(storage_name)
|
||||
end
|
||||
|
|
|
@ -10,7 +10,7 @@ module Gitlab
|
|||
redis.del(*all_storage_keys) unless all_storage_keys.empty?
|
||||
end
|
||||
|
||||
RequestStore.delete(:circuitbreaker_cache)
|
||||
Gitlab::SafeRequestStore.delete(:circuitbreaker_cache)
|
||||
end
|
||||
|
||||
def self.load(cache_key)
|
||||
|
|
|
@ -302,7 +302,7 @@ module Gitlab
|
|||
# Ensures that Gitaly is not being abuse through n+1 misuse etc
|
||||
def self.enforce_gitaly_request_limits(call_site)
|
||||
# Only count limits in request-response environments (not sidekiq for example)
|
||||
return unless RequestStore.active?
|
||||
return unless Gitlab::SafeRequestStore.active?
|
||||
|
||||
# This is this actual number of times this call was made. Used for information purposes only
|
||||
actual_call_count = increment_call_count("gitaly_#{call_site}_actual")
|
||||
|
@ -326,7 +326,7 @@ module Gitlab
|
|||
end
|
||||
|
||||
def self.allow_n_plus_1_calls
|
||||
return yield unless RequestStore.active?
|
||||
return yield unless Gitlab::SafeRequestStore.active?
|
||||
|
||||
begin
|
||||
increment_call_count(:gitaly_call_count_exception_block_depth)
|
||||
|
@ -337,25 +337,25 @@ module Gitlab
|
|||
end
|
||||
|
||||
def self.get_call_count(key)
|
||||
RequestStore.store[key] || 0
|
||||
Gitlab::SafeRequestStore[key] || 0
|
||||
end
|
||||
private_class_method :get_call_count
|
||||
|
||||
def self.increment_call_count(key)
|
||||
RequestStore.store[key] ||= 0
|
||||
RequestStore.store[key] += 1
|
||||
Gitlab::SafeRequestStore[key] ||= 0
|
||||
Gitlab::SafeRequestStore[key] += 1
|
||||
end
|
||||
private_class_method :increment_call_count
|
||||
|
||||
def self.decrement_call_count(key)
|
||||
RequestStore.store[key] -= 1
|
||||
Gitlab::SafeRequestStore[key] -= 1
|
||||
end
|
||||
private_class_method :decrement_call_count
|
||||
|
||||
# Returns an estimate of the number of Gitaly calls made for this
|
||||
# request
|
||||
def self.get_request_count
|
||||
return 0 unless RequestStore.active?
|
||||
return 0 unless Gitlab::SafeRequestStore.active?
|
||||
|
||||
gitaly_migrate_count = get_call_count("gitaly_migrate_actual")
|
||||
gitaly_call_count = get_call_count("gitaly_call_actual")
|
||||
|
@ -372,28 +372,28 @@ module Gitlab
|
|||
end
|
||||
|
||||
def self.reset_counts
|
||||
return unless RequestStore.active?
|
||||
return unless Gitlab::SafeRequestStore.active?
|
||||
|
||||
%w[migrate call].each do |call_site|
|
||||
RequestStore.store["gitaly_#{call_site}_actual"] = 0
|
||||
RequestStore.store["gitaly_#{call_site}_permitted"] = 0
|
||||
Gitlab::SafeRequestStore["gitaly_#{call_site}_actual"] = 0
|
||||
Gitlab::SafeRequestStore["gitaly_#{call_site}_permitted"] = 0
|
||||
end
|
||||
end
|
||||
|
||||
def self.add_call_details(details)
|
||||
id = details.delete(:id)
|
||||
|
||||
return unless id && RequestStore.active? && RequestStore.store[:peek_enabled]
|
||||
return unless id && Gitlab::SafeRequestStore[:peek_enabled]
|
||||
|
||||
RequestStore.store['gitaly_call_details'] ||= {}
|
||||
RequestStore.store['gitaly_call_details'][id] ||= {}
|
||||
RequestStore.store['gitaly_call_details'][id].merge!(details)
|
||||
Gitlab::SafeRequestStore['gitaly_call_details'] ||= {}
|
||||
Gitlab::SafeRequestStore['gitaly_call_details'][id] ||= {}
|
||||
Gitlab::SafeRequestStore['gitaly_call_details'][id].merge!(details)
|
||||
end
|
||||
|
||||
def self.list_call_details
|
||||
return {} unless RequestStore.active? && RequestStore.store[:peek_enabled]
|
||||
return {} unless Gitlab::SafeRequestStore[:peek_enabled]
|
||||
|
||||
RequestStore.store['gitaly_call_details'] || {}
|
||||
Gitlab::SafeRequestStore['gitaly_call_details'] || {}
|
||||
end
|
||||
|
||||
def self.expected_server_version
|
||||
|
@ -431,22 +431,22 @@ module Gitlab
|
|||
|
||||
# Count a stack. Used for n+1 detection
|
||||
def self.count_stack
|
||||
return unless RequestStore.active?
|
||||
return unless Gitlab::SafeRequestStore.active?
|
||||
|
||||
stack_string = Gitlab::Profiler.clean_backtrace(caller).drop(1).join("\n")
|
||||
|
||||
RequestStore.store[:stack_counter] ||= Hash.new
|
||||
Gitlab::SafeRequestStore[:stack_counter] ||= Hash.new
|
||||
|
||||
count = RequestStore.store[:stack_counter][stack_string] || 0
|
||||
RequestStore.store[:stack_counter][stack_string] = count + 1
|
||||
count = Gitlab::SafeRequestStore[:stack_counter][stack_string] || 0
|
||||
Gitlab::SafeRequestStore[:stack_counter][stack_string] = count + 1
|
||||
end
|
||||
private_class_method :count_stack
|
||||
|
||||
# Returns a count for the stack which called Gitaly the most times. Used for n+1 detection
|
||||
def self.max_call_count
|
||||
return 0 unless RequestStore.active?
|
||||
return 0 unless Gitlab::SafeRequestStore.active?
|
||||
|
||||
stack_counter = RequestStore.store[:stack_counter]
|
||||
stack_counter = Gitlab::SafeRequestStore[:stack_counter]
|
||||
return 0 unless stack_counter
|
||||
|
||||
stack_counter.values.max
|
||||
|
@ -455,9 +455,9 @@ module Gitlab
|
|||
|
||||
# Returns the stacks that calls Gitaly the most times. Used for n+1 detection
|
||||
def self.max_stacks
|
||||
return nil unless RequestStore.active?
|
||||
return nil unless Gitlab::SafeRequestStore.active?
|
||||
|
||||
stack_counter = RequestStore.store[:stack_counter]
|
||||
stack_counter = Gitlab::SafeRequestStore[:stack_counter]
|
||||
return nil unless stack_counter
|
||||
|
||||
max = max_call_count
|
||||
|
|
|
@ -240,22 +240,23 @@ module Gitlab
|
|||
end
|
||||
|
||||
def find_commit(revision)
|
||||
if RequestStore.active?
|
||||
# We don't use RequeStstore.fetch(key) { ... } directly because `revision`
|
||||
# can be a branch name, so we can't use it as a key as it could point
|
||||
# to another commit later on (happens a lot in tests).
|
||||
if Gitlab::SafeRequestStore.active?
|
||||
# We don't use Gitlab::SafeRequestStore.fetch(key) { ... } directly
|
||||
# because `revision` can be a branch name, so we can't use it as a key
|
||||
# as it could point to another commit later on (happens a lot in
|
||||
# tests).
|
||||
key = {
|
||||
storage: @gitaly_repo.storage_name,
|
||||
relative_path: @gitaly_repo.relative_path,
|
||||
commit_id: revision
|
||||
}
|
||||
return RequestStore[key] if RequestStore.exist?(key)
|
||||
return Gitlab::SafeRequestStore[key] if Gitlab::SafeRequestStore.exist?(key)
|
||||
|
||||
commit = call_find_commit(revision)
|
||||
return unless commit
|
||||
|
||||
key[:commit_id] = commit.id
|
||||
RequestStore[key] = commit
|
||||
Gitlab::SafeRequestStore[key] = commit
|
||||
else
|
||||
call_find_commit(revision)
|
||||
end
|
||||
|
|
|
@ -30,7 +30,7 @@ module Gitlab
|
|||
end
|
||||
|
||||
def self.build
|
||||
RequestStore[self.cache_key] ||= new(self.full_log_path)
|
||||
Gitlab::SafeRequestStore[self.cache_key] ||= new(self.full_log_path)
|
||||
end
|
||||
|
||||
def self.full_log_path
|
||||
|
|
|
@ -2,7 +2,7 @@ module Gitlab
|
|||
class RequestContext
|
||||
class << self
|
||||
def client_ip
|
||||
RequestStore[:client_ip]
|
||||
Gitlab::SafeRequestStore[:client_ip]
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -13,7 +13,7 @@ module Gitlab
|
|||
def call(env)
|
||||
req = Rack::Request.new(env)
|
||||
|
||||
RequestStore[:client_ip] = req.ip
|
||||
Gitlab::SafeRequestStore[:client_ip] = req.ip
|
||||
|
||||
@app.call(env)
|
||||
end
|
||||
|
|
|
@ -10,7 +10,7 @@ module Gitlab
|
|||
end
|
||||
|
||||
def temporarily_allowed?(key)
|
||||
if RequestStore.active?
|
||||
if Gitlab::SafeRequestStore.active?
|
||||
temporarily_allow_request_store[key] > 0
|
||||
else
|
||||
TEMPORARILY_ALLOW_MUTEX.synchronize do
|
||||
|
@ -26,11 +26,11 @@ module Gitlab
|
|||
end
|
||||
|
||||
def temporarily_allow_request_store
|
||||
RequestStore[:temporarily_allow] ||= Hash.new(0)
|
||||
Gitlab::SafeRequestStore[:temporarily_allow] ||= Hash.new(0)
|
||||
end
|
||||
|
||||
def temporarily_allow_add(key, value)
|
||||
if RequestStore.active?
|
||||
if Gitlab::SafeRequestStore.active?
|
||||
temporarily_allow_request_store[key] += value
|
||||
else
|
||||
TEMPORARILY_ALLOW_MUTEX.synchronize do
|
||||
|
|
|
@ -79,13 +79,9 @@ describe Banzai::Filter::ExternalIssueReferenceFilter do
|
|||
expect(link).to eq helper.url_for_issue(issue_id, project, only_path: true)
|
||||
end
|
||||
|
||||
context 'with RequestStore enabled' do
|
||||
context 'with RequestStore enabled', :request_store do
|
||||
let(:reference_filter) { HTML::Pipeline.new([described_class]) }
|
||||
|
||||
before do
|
||||
allow(RequestStore).to receive(:active?).and_return(true)
|
||||
end
|
||||
|
||||
it 'queries the collection on the first call' do
|
||||
expect_any_instance_of(Project).to receive(:default_issues_tracker?).once.and_call_original
|
||||
expect_any_instance_of(Project).to receive(:external_issue_reference_pattern).once.and_call_original
|
||||
|
|
|
@ -263,11 +263,10 @@ describe Banzai::ReferenceParser::BaseParser do
|
|||
end
|
||||
end
|
||||
|
||||
context 'with RequestStore enabled' do
|
||||
context 'with RequestStore enabled', :request_store do
|
||||
before do
|
||||
cache = Hash.new { |hash, key| hash[key] = {} }
|
||||
|
||||
allow(RequestStore).to receive(:active?).and_return(true)
|
||||
allow(subject).to receive(:collection_cache).and_return(cache)
|
||||
end
|
||||
|
||||
|
|
|
@ -4,11 +4,7 @@ describe Gitlab::Git::HookEnv do
|
|||
let(:gl_repository) { 'project-123' }
|
||||
|
||||
describe ".set" do
|
||||
context 'with RequestStore.store disabled' do
|
||||
before do
|
||||
allow(RequestStore).to receive(:active?).and_return(false)
|
||||
end
|
||||
|
||||
context 'with RequestStore disabled' do
|
||||
it 'does not store anything' do
|
||||
described_class.set(gl_repository, GIT_OBJECT_DIRECTORY_RELATIVE: 'foo')
|
||||
|
||||
|
@ -16,11 +12,7 @@ describe Gitlab::Git::HookEnv do
|
|||
end
|
||||
end
|
||||
|
||||
context 'with RequestStore.store enabled' do
|
||||
before do
|
||||
allow(RequestStore).to receive(:active?).and_return(true)
|
||||
end
|
||||
|
||||
context 'with RequestStore enabled', :request_store do
|
||||
it 'whitelist some `GIT_*` variables and stores them using RequestStore' do
|
||||
described_class.set(
|
||||
gl_repository,
|
||||
|
@ -41,9 +33,8 @@ describe Gitlab::Git::HookEnv do
|
|||
end
|
||||
|
||||
describe ".all" do
|
||||
context 'with RequestStore.store enabled' do
|
||||
context 'with RequestStore enabled', :request_store do
|
||||
before do
|
||||
allow(RequestStore).to receive(:active?).and_return(true)
|
||||
described_class.set(
|
||||
gl_repository,
|
||||
GIT_OBJECT_DIRECTORY_RELATIVE: 'foo',
|
||||
|
@ -60,7 +51,7 @@ describe Gitlab::Git::HookEnv do
|
|||
end
|
||||
|
||||
describe ".to_env_hash" do
|
||||
context 'with RequestStore.store enabled' do
|
||||
context 'with RequestStore enabled', :request_store do
|
||||
using RSpec::Parameterized::TableSyntax
|
||||
|
||||
let(:key) { 'GIT_OBJECT_DIRECTORY_RELATIVE' }
|
||||
|
@ -76,7 +67,6 @@ describe Gitlab::Git::HookEnv do
|
|||
|
||||
with_them do
|
||||
before do
|
||||
allow(RequestStore).to receive(:active?).and_return(true)
|
||||
described_class.set(gl_repository, key.to_sym => input)
|
||||
end
|
||||
|
||||
|
@ -92,7 +82,7 @@ describe Gitlab::Git::HookEnv do
|
|||
end
|
||||
|
||||
describe 'thread-safety' do
|
||||
context 'with RequestStore.store enabled' do
|
||||
context 'with RequestStore enabled', :request_store do
|
||||
before do
|
||||
allow(RequestStore).to receive(:active?).and_return(true)
|
||||
described_class.set(gl_repository, GIT_OBJECT_DIRECTORY_RELATIVE: 'foo')
|
||||
|
|
|
@ -65,7 +65,7 @@ describe Commit do
|
|||
|
||||
key = "Commit:author:#{commit.author_email.downcase}"
|
||||
|
||||
expect(RequestStore.store[key]).to eq(user)
|
||||
expect(Gitlab::SafeRequestStore[key]).to eq(user)
|
||||
expect(commit.author).to eq(user)
|
||||
end
|
||||
|
||||
|
|
Loading…
Reference in New Issue