Use a null object with RequestStore

Makes it easier and safer to use RequestStore because you don't need to
check `RequestStore.active?` before using it. You just have to use
`Gitlab::SafeRequestStore` instead.

4 files changed, 384 insertions, 0 deletions

diff --git a/lib/gitlab/null_request_store.rb b/lib/gitlab/null_request_store.rb
new file mode 100644
index 00000000000..8db331dcb9f
--- /dev/null
+++ b/lib/gitlab/null_request_store.rb
@@ -0,0 +1,41 @@
+# 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
diff --git a/lib/gitlab/safe_request_store.rb b/lib/gitlab/safe_request_store.rb
new file mode 100644
index 00000000000..4e82353adb6
--- /dev/null
+++ b/lib/gitlab/safe_request_store.rb
@@ -0,0 +1,23 @@
+# 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
diff --git a/spec/lib/gitlab/null_request_store_spec.rb b/spec/lib/gitlab/null_request_store_spec.rb
new file mode 100644
index 00000000000..c023dac53ad
--- /dev/null
+++ b/spec/lib/gitlab/null_request_store_spec.rb
@@ -0,0 +1,75 @@
+# 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
diff --git a/spec/lib/gitlab/safe_request_store_spec.rb b/spec/lib/gitlab/safe_request_store_spec.rb
new file mode 100644
index 00000000000..27766fa0eda
--- /dev/null
+++ b/spec/lib/gitlab/safe_request_store_spec.rb
@@ -0,0 +1,245 @@
+# 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