Merge branch 'mk/improve-usage-of-request-store' into 'master'

Resolve "Provide NullStore for RequestStore"

Closes #51718

See merge request gitlab-org/gitlab-ce!21848

app/controllers/concerns/with_performance_bar.rb

@@ -10,11 +10,7 @@ module WithPerformanceBar
   def peek_enabled?
     return false unless Gitlab::PerformanceBar.enabled?(current_user)
 
-    if RequestStore.active?
-      RequestStore.fetch(:peek_enabled) { cookie_or_default_value }
-    else
-      cookie_or_default_value
-    end
+    Gitlab::SafeRequestStore.fetch(:peek_enabled) { cookie_or_default_value }
   end
 
   private

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
 

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

app/models/concerns/cacheable_attributes.rb

@@ -27,11 +27,7 @@ module CacheableAttributes
     end
 
     def cached
-      if RequestStore.active?
-        RequestStore[:"#{name}_cached_attributes"] ||= retrieve_from_cache
-      else
-        retrieve_from_cache
-      end
+      Gitlab::SafeRequestStore[:"#{name}_cached_attributes"] ||= retrieve_from_cache
     end
 
     def retrieve_from_cache

app/models/legacy_diff_note.rb

@@ -20,11 +20,7 @@ class LegacyDiffNote < Note
   end
 
   def project_repository
-    if RequestStore.active?
-      RequestStore.fetch("project:#{project_id}:repository") { self.project.repository }
-    else
-      self.project.repository
-    end
+    Gitlab::SafeRequestStore.fetch("project:#{project_id}:repository") { self.project.repository }
   end
 
   def diff_file_hash

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

app/models/project.rb

@@ -2266,11 +2266,7 @@ class Project < ActiveRecord::Base
       end
     end
 
-    if RequestStore.active?
-      RequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_collaboration") do
-        check_access.call
-      end
-    else
+    Gitlab::SafeRequestStore.fetch("project-#{id}:branch-#{branch_name}:user-#{user.id}:branch_allows_collaboration") do
       check_access.call
     end
   end

doc/development/merge_request_performance_guidelines.md

@@ -168,6 +168,7 @@ user objects for every username we can remove the need for running the same
 query for every mention of `@alice`.
 
 Caching data per transaction can be done using
-[RequestStore](https://github.com/steveklabnik/request_store). Caching data in
-Redis can be done using [Rails' caching
+[RequestStore](https://github.com/steveklabnik/request_store) (use
+`Gitlab::SafeRequestStore` to avoid having to remember to check
+`RequestStore.active?`). Caching data in Redis can be done using [Rails' caching
 system](http://guides.rubyonrails.org/caching_with_rails.html).

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

lib/banzai/filter/external_issue_reference_filter.rb

@@ -97,9 +97,7 @@ module Banzai
       private
 
       def external_issues_cached(attribute)
-        return project.public_send(attribute) unless RequestStore.active? # rubocop:disable GitlabSecurity/PublicSend
-
-        cached_attributes = RequestStore[:banzai_external_issues_tracker_attributes] ||= Hash.new { |h, k| h[k] = {} }
+        cached_attributes = Gitlab::SafeRequestStore[:banzai_external_issues_tracker_attributes] ||= Hash.new { |h, k| h[k] = {} }
         cached_attributes[project.id][attribute] = project.public_send(attribute) if cached_attributes[project.id][attribute].nil? # rubocop:disable GitlabSecurity/PublicSend
         cached_attributes[project.id][attribute]
       end

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

lib/banzai/request_store_reference_cache.rb

 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
 

lib/feature.rb

@@ -28,11 +28,7 @@ class Feature
     end
 
     def persisted_names
-      if RequestStore.active?
-        RequestStore[:flipper_persisted_names] ||= FlipperFeature.feature_names
-      else
-        FlipperFeature.feature_names
-      end
+      Gitlab::SafeRequestStore[:flipper_persisted_names] ||= FlipperFeature.feature_names
     end
 
     def persisted?(feature)
@@ -76,11 +72,7 @@ class Feature
     end
 
     def flipper
-      if RequestStore.active?
-        RequestStore[:flipper] ||= build_flipper_instance
-      else
-        @flipper ||= build_flipper_instance
-      end
+      @flipper ||= (Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance)
     end
 
     def build_flipper_instance

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")}"

lib/gitlab/current_settings.rb

@@ -2,11 +2,7 @@ module Gitlab
   module CurrentSettings
     class << self
       def current_application_settings
-        if RequestStore.active?
-          RequestStore.fetch(:current_application_settings) { ensure_application_settings! }
-        else
-          ensure_application_settings!
-        end
+        Gitlab::SafeRequestStore.fetch(:current_application_settings) { ensure_application_settings! }
       end
 
       def fake_application_settings(attributes = {})

lib/gitlab/diff/position.rb

@@ -101,18 +101,14 @@ module Gitlab
         return @diff_file if defined?(@diff_file)
 
         @diff_file = begin
-          if RequestStore.active?
-            key = {
-              project_id: repository.project.id,
-              start_sha: start_sha,
-              head_sha: head_sha,
-              path: file_path
-            }
-
-            RequestStore.fetch(key) { find_diff_file(repository) }
-          else
-            find_diff_file(repository)
-          end
+          key = {
+            project_id: repository.project.id,
+            start_sha: start_sha,
+            head_sha: head_sha,
+            path: file_path
+          }
+
+          Gitlab::SafeRequestStore.fetch(key) { find_diff_file(repository) }
         end
       end
 

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

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
 

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

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)

lib/gitlab/git/wiki.rb

@@ -115,11 +115,7 @@ module Gitlab
       def version(commit_id)
         commit_find_proc = -> { Gitlab::Git::Commit.find(@repository, commit_id) }
 
-        if RequestStore.active?
-          RequestStore.fetch([:wiki_version_commit, commit_id]) { commit_find_proc.call }
-        else
-          commit_find_proc.call
-        end
+        Gitlab::SafeRequestStore.fetch([:wiki_version_commit, commit_id]) { commit_find_proc.call }
       end
 
       def assert_type!(object, klass)

lib/gitlab/gitaly_client.rb

@@ -316,7 +316,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")
@@ -340,7 +340,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)
@@ -351,25 +351,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")
@@ -386,28 +386,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
@@ -445,22 +445,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
@@ -469,9 +469,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

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

lib/gitlab/issuables_count_for_state.rb

 module Gitlab
   # Class for counting and caching the number of issuables per state.
   class IssuablesCountForState
-    # The name of the RequestStore cache key.
+    # The name of the Gitlab::SafeRequestStore cache key.
     CACHE_KEY = :issuables_count_for_state
 
     # The state values that can be safely casted to a Symbol.
@@ -10,12 +10,7 @@ module Gitlab
     # finder - The finder class to use for retrieving the issuables.
     def initialize(finder)
       @finder = finder
-      @cache =
-        if RequestStore.active?
-          RequestStore[CACHE_KEY] ||= initialize_cache
-        else
-          initialize_cache
-        end
+      @cache = Gitlab::SafeRequestStore[CACHE_KEY] ||= initialize_cache
     end
 
     def for_state_or_opened(state = nil)

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

lib/gitlab/null_request_store.rb

+# frozen_string_literal: true
+
+# Used by Gitlab::SafeRequestStore
+module Gitlab
+  # The methods `begin!`, `clear!`, and `end!` are not defined because they
+  # should only be called directly on `RequestStore`.
+  class NullRequestStore
+    def store
+      {}
+    end
+
+    def active?
+    end
+
+    def read(key)
+    end
+
+    def [](key)
+    end
+
+    def write(key, value)
+      value
+    end
+
+    def []=(key, value)
+      value
+    end
+
+    def exist?(key)
+      false
+    end
+
+    def fetch(key, &block)
+      yield
+    end
+
+    def delete(key, &block)
+      yield(key) if block_given?
+    end
+  end
+end

lib/gitlab/performance_bar/peek_query_tracker.rb

@@ -23,7 +23,7 @@ module Gitlab
         end
 
         subscribe('sql.active_record') do |_, start, finish, _, data|
-          if RequestStore.active? && RequestStore.store[:peek_enabled]
+          if Gitlab::SafeRequestStore.store[:peek_enabled]
             # data[:cached] is only available starting from Rails 5.1.0
             # https://github.com/rails/rails/blob/v5.1.0/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb#L113
             # Before that, data[:name] was set to 'CACHE'

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

lib/gitlab/safe_request_store.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module SafeRequestStore
+    NULL_STORE = Gitlab::NullRequestStore.new
+
+    class << self
+      # These methods should always run directly against RequestStore
+      delegate :clear!, :begin!, :end!, :active?, to: :RequestStore
+
+      # These methods will run against NullRequestStore if RequestStore is disabled
+      delegate :read, :[], :write, :[]=, :exist?, :fetch, :delete, to: :store
+    end
+
+    def self.store
+      if RequestStore.active?
+        RequestStore
+      else
+        NULL_STORE
+      end
+    end
+  end
+end

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

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

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
 

spec/lib/feature_spec.rb

@@ -91,7 +91,11 @@ describe Feature do
   end
 
   describe '.flipper' do
-    shared_examples 'a memoized Flipper instance' do
+    before do
+      described_class.instance_variable_set(:@flipper, nil)
+    end
+
+    context 'when request store is inactive' do
       it 'memoizes the Flipper instance' do
         expect(Flipper).to receive(:new).once.and_call_original
 
@@ -101,16 +105,14 @@ describe Feature do
       end
     end
 
-    context 'when request store is inactive' do
-      before do
+    context 'when request store is active', :request_store do
+      it 'memoizes the Flipper instance' do
+        expect(Flipper).to receive(:new).once.and_call_original
+
+        described_class.flipper
         described_class.instance_variable_set(:@flipper, nil)
+        described_class.flipper
       end
-
-      it_behaves_like 'a memoized Flipper instance'
-    end
-
-    context 'when request store is inactive', :request_store do
-      it_behaves_like 'a memoized Flipper instance'
     end
   end
 

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')

spec/lib/gitlab/null_request_store_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::NullRequestStore do
+  let(:null_store) { described_class.new }
+
+  describe '#store' do
+    it 'returns an empty hash' do
+      expect(null_store.store).to eq({})
+    end
+  end
+
+  describe '#active?' do
+    it 'returns falsey' do
+      expect(null_store.active?).to be_falsey
+    end
+  end
+
+  describe '#read' do
+    it 'returns nil' do
+      expect(null_store.read('foo')).to be nil
+    end
+  end
+
+  describe '#[]' do
+    it 'returns nil' do
+      expect(null_store['foo']).to be nil
+    end
+  end
+
+  describe '#write' do
+    it 'returns the same value' do
+      expect(null_store.write('key', 'value')).to eq('value')
+    end
+  end
+
+  describe '#[]=' do
+    it 'returns the same value' do
+      expect(null_store['key'] = 'value').to eq('value')
+    end
+  end
+
+  describe '#exist?' do
+    it 'returns falsey' do
+      expect(null_store.exist?('foo')).to be_falsey
+    end
+  end
+
+  describe '#fetch' do
+    it 'returns the block result' do
+      expect(null_store.fetch('key') { 'block result' }).to eq('block result')
+    end
+  end
+
+  describe '#delete' do
+    context 'when a block is given' do
+      it 'yields the key to the block' do
+        expect do |b|
+          null_store.delete('foo', &b)
+        end.to yield_with_args('foo')
+      end
+
+      it 'returns the block result' do
+        expect(null_store.delete('foo') { |key| 'block result' }).to eq('block result')
+      end
+    end
+
+    context 'when a block is not given' do
+      it 'returns nil' do
+        expect(null_store.delete('foo')).to be nil
+      end
+    end
+  end
+end

spec/lib/gitlab/safe_request_store_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::SafeRequestStore do
+  describe '.store' do
+    context 'when RequestStore is active', :request_store do
+      it 'uses RequestStore' do
+        expect(described_class.store).to eq(RequestStore)
+      end
+    end
+
+    context 'when RequestStore is NOT active' do
+      it 'does not use RequestStore' do
+        expect(described_class.store).to be_a(Gitlab::NullRequestStore)
+      end
+    end
+  end
+
+  describe '.begin!' do
+    context 'when RequestStore is active', :request_store do
+      it 'uses RequestStore' do
+        expect(RequestStore).to receive(:begin!)
+
+        described_class.begin!
+      end
+    end
+
+    context 'when RequestStore is NOT active' do
+      it 'uses RequestStore' do
+        expect(RequestStore).to receive(:begin!)
+
+        described_class.begin!
+      end
+    end
+  end
+
+  describe '.clear!' do
+    context 'when RequestStore is active', :request_store do
+      it 'uses RequestStore' do
+        expect(RequestStore).to receive(:clear!).twice.and_call_original
+
+        described_class.clear!
+      end
+    end
+
+    context 'when RequestStore is NOT active' do
+      it 'uses RequestStore' do
+        expect(RequestStore).to receive(:clear!).and_call_original
+
+        described_class.clear!
+      end
+    end
+  end
+
+  describe '.end!' do
+    context 'when RequestStore is active', :request_store do
+      it 'uses RequestStore' do
+        expect(RequestStore).to receive(:end!).twice.and_call_original
+
+        described_class.end!
+      end
+    end
+
+    context 'when RequestStore is NOT active' do
+      it 'uses RequestStore' do
+        expect(RequestStore).to receive(:end!).and_call_original
+
+        described_class.end!
+      end
+    end
+  end
+
+  describe '.write' do
+    context 'when RequestStore is active', :request_store do
+      it 'uses RequestStore' do
+        expect do
+          described_class.write('foo', true)
+        end.to change { described_class.read('foo') }.from(nil).to(true)
+      end
+    end
+
+    context 'when RequestStore is NOT active' do
+      it 'does not use RequestStore' do
+        expect do
+          described_class.write('foo', true)
+        end.not_to change { described_class.read('foo') }.from(nil)
+      end
+    end
+  end
+
+  describe '.[]=' do
+    context 'when RequestStore is active', :request_store do
+      it 'uses RequestStore' do
+        expect do
+          described_class['foo'] = true
+        end.to change { described_class.read('foo') }.from(nil).to(true)
+      end
+    end
+
+    context 'when RequestStore is NOT active' do
+      it 'does not use RequestStore' do
+        expect do
+          described_class['foo'] = true
+        end.not_to change { described_class.read('foo') }.from(nil)
+      end
+    end
+  end
+
+  describe '.read' do
+    context 'when RequestStore is active', :request_store do
+      it 'uses RequestStore' do
+        expect do
+          RequestStore.write('foo', true)
+        end.to change { described_class.read('foo') }.from(nil).to(true)
+      end
+    end
+
+    context 'when RequestStore is NOT active' do
+      it 'does not use RequestStore' do
+        expect do
+          RequestStore.write('foo', true)
+        end.not_to change { described_class.read('foo') }.from(nil)
+
+        RequestStore.clear! # Clean up
+      end
+    end
+  end
+
+  describe '.[]' do
+    context 'when RequestStore is active', :request_store do
+      it 'uses RequestStore' do
+        expect do
+          RequestStore.write('foo', true)
+        end.to change { described_class['foo'] }.from(nil).to(true)
+      end
+    end
+
+    context 'when RequestStore is NOT active' do
+      it 'does not use RequestStore' do
+        expect do
+          RequestStore.write('foo', true)
+        end.not_to change { described_class['foo'] }.from(nil)
+
+        RequestStore.clear! # Clean up
+      end
+    end
+  end
+
+  describe '.exist?' do
+    context 'when RequestStore is active', :request_store do
+      it 'uses RequestStore' do
+        expect do
+          RequestStore.write('foo', 'not nil')
+        end.to change { described_class.exist?('foo') }.from(false).to(true)
+      end
+    end
+
+    context 'when RequestStore is NOT active' do
+      it 'does not use RequestStore' do
+        expect do
+          RequestStore.write('foo', 'not nil')
+        end.not_to change { described_class.exist?('foo') }.from(false)
+
+        RequestStore.clear! # Clean up
+      end
+    end
+  end
+
+  describe '.fetch' do
+    context 'when RequestStore is active', :request_store do
+      it 'uses RequestStore' do
+        expect do
+          described_class.fetch('foo') { 'block result' }
+        end.to change { described_class.read('foo') }.from(nil).to('block result')
+      end
+    end
+
+    context 'when RequestStore is NOT active' do
+      it 'does not use RequestStore' do
+        RequestStore.clear! # Ensure clean
+
+        expect do
+          described_class.fetch('foo') { 'block result' }
+        end.not_to change { described_class.read('foo') }.from(nil)
+
+        RequestStore.clear! # Clean up
+      end
+    end
+  end
+
+  describe '.delete' do
+    context 'when RequestStore is active', :request_store do
+      it 'uses RequestStore' do
+        described_class.write('foo', true)
+
+        expect do
+          described_class.delete('foo')
+        end.to change { described_class.read('foo') }.from(true).to(nil)
+      end
+
+      context 'when given a block and the key exists' do
+        it 'does not execute the block' do
+          described_class.write('foo', true)
+
+          expect do |b|
+            described_class.delete('foo', &b)
+          end.not_to yield_control
+        end
+      end
+
+      context 'when given a block and the key does not exist' do
+        it 'yields the key and returns the block result' do
+          result = described_class.delete('foo') { |key| "#{key} block result" }
+
+          expect(result).to eq('foo block result')
+        end
+      end
+    end
+
+    context 'when RequestStore is NOT active' do
+      around do |example|
+        RequestStore.write('foo', true)
+
+        example.run
+
+        RequestStore.clear! # Clean up
+      end
+
+      it 'does not use RequestStore' do
+        expect do
+          described_class.delete('foo')
+        end.not_to change { RequestStore.read('foo') }.from(true)
+      end
+
+      context 'when given a block' do
+        it 'yields the key and returns the block result' do
+          result = described_class.delete('foo') { |key| "#{key} block result" }
+
+          expect(result).to eq('foo block result')
+        end
+      end
+    end
+  end
+end

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