From a54a5d9f39df505fe7c68e14c693553bd29bd725 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 20 Sep 2018 15:40:15 -0700 Subject: Use `Gitlab::SafeRequestStore` in more places MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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?`. --- app/models/ability.rb | 2 +- app/models/concerns/bulk_member_access_load.rb | 6 +-- app/models/namespace.rb | 4 +- lib/banzai/filter/abstract_reference_filter.rb | 4 +- lib/banzai/reference_parser/base_parser.rb | 4 +- lib/banzai/request_store_reference_cache.rb | 4 +- lib/gitlab/cache/request_cache.rb | 4 +- lib/gitlab/favicon.rb | 2 +- lib/gitlab/git/hook_env.rb | 10 ++--- lib/gitlab/git/storage/circuit_breaker.rb | 2 +- lib/gitlab/git/storage/failure_info.rb | 2 +- lib/gitlab/gitaly_client.rb | 48 +++++++++++----------- lib/gitlab/gitaly_client/commit_service.rb | 13 +++--- lib/gitlab/logger.rb | 2 +- lib/gitlab/request_context.rb | 4 +- lib/gitlab/temporarily_allow.rb | 6 +-- .../filter/external_issue_reference_filter_spec.rb | 6 +-- .../banzai/reference_parser/base_parser_spec.rb | 3 +- spec/lib/gitlab/git/hook_env_spec.rb | 20 +++------ spec/models/commit_spec.rb | 2 +- 20 files changed, 67 insertions(+), 81 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index a853106e5bd..1466407d0d1 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -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 diff --git a/app/models/concerns/bulk_member_access_load.rb b/app/models/concerns/bulk_member_access_load.rb index c4346d5dd17..041ed3755e0 100644 --- a/app/models/concerns/bulk_member_access_load.rb +++ b/app/models/concerns/bulk_member_access_load.rb @@ -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 diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 0289f29211d..c54be778d1b 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -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 diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb index ad0806df8e6..4764f8e1e19 100644 --- a/lib/banzai/filter/abstract_reference_filter.rb +++ b/lib/banzai/filter/abstract_reference_filter.rb @@ -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 diff --git a/lib/banzai/reference_parser/base_parser.rb b/lib/banzai/reference_parser/base_parser.rb index 68752f5bb5a..3ab154a7b1c 100644 --- a/lib/banzai/reference_parser/base_parser.rb +++ b/lib/banzai/reference_parser/base_parser.rb @@ -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 diff --git a/lib/banzai/request_store_reference_cache.rb b/lib/banzai/request_store_reference_cache.rb index 426131442a2..9a9704f9837 100644 --- a/lib/banzai/request_store_reference_cache.rb +++ b/lib/banzai/request_store_reference_cache.rb @@ -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 diff --git a/lib/gitlab/cache/request_cache.rb b/lib/gitlab/cache/request_cache.rb index 671b8e7e1b1..b96e161a5b6 100644 --- a/lib/gitlab/cache/request_cache.rb +++ b/lib/gitlab/cache/request_cache.rb @@ -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")}" diff --git a/lib/gitlab/favicon.rb b/lib/gitlab/favicon.rb index 4850a6c0430..050a1ad3a0b 100644 --- a/lib/gitlab/favicon.rb +++ b/lib/gitlab/favicon.rb @@ -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 diff --git a/lib/gitlab/git/hook_env.rb b/lib/gitlab/git/hook_env.rb index 455e8451c10..620568d8817 100644 --- a/lib/gitlab/git/hook_env.rb +++ b/lib/gitlab/git/hook_env.rb @@ -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 diff --git a/lib/gitlab/git/storage/circuit_breaker.rb b/lib/gitlab/git/storage/circuit_breaker.rb index 62427ac9cc4..fcee9ae566c 100644 --- a/lib/gitlab/git/storage/circuit_breaker.rb +++ b/lib/gitlab/git/storage/circuit_breaker.rb @@ -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 diff --git a/lib/gitlab/git/storage/failure_info.rb b/lib/gitlab/git/storage/failure_info.rb index 387279c110d..1d28a850049 100644 --- a/lib/gitlab/git/storage/failure_info.rb +++ b/lib/gitlab/git/storage/failure_info.rb @@ -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) diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index 12307338972..a7b67b2786d 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -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 diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index 6c95abdcb4b..07e5e204b68 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -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 diff --git a/lib/gitlab/logger.rb b/lib/gitlab/logger.rb index e58927a40b9..3d7c049c17f 100644 --- a/lib/gitlab/logger.rb +++ b/lib/gitlab/logger.rb @@ -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 diff --git a/lib/gitlab/request_context.rb b/lib/gitlab/request_context.rb index fef536ecb0b..8562d4515aa 100644 --- a/lib/gitlab/request_context.rb +++ b/lib/gitlab/request_context.rb @@ -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 diff --git a/lib/gitlab/temporarily_allow.rb b/lib/gitlab/temporarily_allow.rb index 880e55f71df..8d7dacc6c54 100644 --- a/lib/gitlab/temporarily_allow.rb +++ b/lib/gitlab/temporarily_allow.rb @@ -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 diff --git a/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb b/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb index d9018a7e4fe..0d0554a2259 100644 --- a/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/external_issue_reference_filter_spec.rb @@ -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 diff --git a/spec/lib/banzai/reference_parser/base_parser_spec.rb b/spec/lib/banzai/reference_parser/base_parser_spec.rb index 4e6e8eca38a..c6e9fc414a1 100644 --- a/spec/lib/banzai/reference_parser/base_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/base_parser_spec.rb @@ -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 diff --git a/spec/lib/gitlab/git/hook_env_spec.rb b/spec/lib/gitlab/git/hook_env_spec.rb index e6aa5ad8c90..5e49ea6da7a 100644 --- a/spec/lib/gitlab/git/hook_env_spec.rb +++ b/spec/lib/gitlab/git/hook_env_spec.rb @@ -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') diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index d5f88e930d4..a6957095166 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -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 -- cgit v1.2.1