From 45cf64c827270d66a88d483bb3f9043a90301255 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Thu, 20 Sep 2018 11:49:58 -0700 Subject: [PATCH] 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. --- lib/gitlab/null_request_store.rb | 41 ++++ lib/gitlab/safe_request_store.rb | 23 ++ spec/lib/gitlab/null_request_store_spec.rb | 75 +++++++ spec/lib/gitlab/safe_request_store_spec.rb | 245 +++++++++++++++++++++ 4 files changed, 384 insertions(+) create mode 100644 lib/gitlab/null_request_store.rb create mode 100644 lib/gitlab/safe_request_store.rb create mode 100644 spec/lib/gitlab/null_request_store_spec.rb create mode 100644 spec/lib/gitlab/safe_request_store_spec.rb 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