Cache `Repository#exists?` false in RequestStore

* Only truthy values are cached in Redis.
* All values are cached in RequestStore and in an instance variable.

4 files changed, 212 insertions, 1 deletions

diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb
index 0d49033c691..5c7415a318d 100644
--- a/spec/controllers/projects/pipelines_controller_spec.rb
+++ b/spec/controllers/projects/pipelines_controller_spec.rb
@@ -90,6 +90,11 @@ describe Projects::PipelinesController do
 
     context 'when performing gitaly calls', :request_store do
       it 'limits the Gitaly requests' do
+        # Isolate from test preparation (Repository#exists? is also cached in RequestStore)
+        RequestStore.end!
+        RequestStore.clear!
+        RequestStore.begin!
+
         expect { get_pipelines_index_json }
           .to change { Gitlab::GitalyClient.get_request_count }.by(2)
       end
diff --git a/spec/lib/gitlab/repository_cache_adapter_spec.rb b/spec/lib/gitlab/repository_cache_adapter_spec.rb
index 703e73f1a9b..0295138fc3a 100644
--- a/spec/lib/gitlab/repository_cache_adapter_spec.rb
+++ b/spec/lib/gitlab/repository_cache_adapter_spec.rb
@@ -65,6 +65,86 @@ describe Gitlab::RepositoryCacheAdapter do
     end
   end
 
+  describe '#cache_method_output_asymmetrically', :use_clean_rails_memory_store_caching, :request_store do
+    let(:request_store_cache) { repository.send(:request_store_cache) }
+
+    context 'with a non-existing repository' do
+      let(:project) { create(:project) } # No repository
+      let(:object) { double }
+
+      subject do
+        repository.cache_method_output_asymmetrically(:cats) do
+          object.cats_call_stub
+        end
+      end
+
+      it 'returns the output of the original method' do
+        expect(object).to receive(:cats_call_stub).and_return('output')
+
+        expect(subject).to eq('output')
+      end
+    end
+
+    context 'with a method throwing a non-existing-repository error' do
+      subject do
+        repository.cache_method_output_asymmetrically(:cats) do
+          raise Gitlab::Git::Repository::NoRepository
+        end
+      end
+
+      it 'returns nil' do
+        expect(subject).to eq(nil)
+      end
+
+      it 'does not cache the data' do
+        subject
+
+        expect(repository.instance_variable_defined?(:@cats)).to eq(false)
+        expect(cache.exist?(:cats)).to eq(false)
+      end
+    end
+
+    context 'with an existing repository' do
+      let(:object) { double }
+
+      context 'when it returns truthy' do
+        before do
+          expect(object).to receive(:cats).once.and_return('truthy output')
+        end
+
+        it 'caches the output in RequestStore' do
+          expect do
+            repository.cache_method_output_asymmetrically(:cats) { object.cats }
+          end.to change { request_store_cache.read(:cats) }.from(nil).to('truthy output')
+        end
+
+        it 'caches the output in RepositoryCache' do
+          expect do
+            repository.cache_method_output_asymmetrically(:cats) { object.cats }
+          end.to change { cache.read(:cats) }.from(nil).to('truthy output')
+        end
+      end
+
+      context 'when it returns false' do
+        before do
+          expect(object).to receive(:cats).once.and_return(false)
+        end
+
+        it 'caches the output in RequestStore' do
+          expect do
+            repository.cache_method_output_asymmetrically(:cats) { object.cats }
+          end.to change { request_store_cache.read(:cats) }.from(nil).to(false)
+        end
+
+        it 'does NOT cache the output in RepositoryCache' do
+          expect do
+            repository.cache_method_output_asymmetrically(:cats) { object.cats }
+          end.not_to change { cache.read(:cats) }.from(nil)
+        end
+      end
+    end
+  end
+
   describe '#memoize_method_output' do
     let(:fallback) { 10 }
 
diff --git a/spec/lib/gitlab/repository_cache_spec.rb b/spec/lib/gitlab/repository_cache_spec.rb
index fc259cf1208..741ee12633f 100644
--- a/spec/lib/gitlab/repository_cache_spec.rb
+++ b/spec/lib/gitlab/repository_cache_spec.rb
@@ -47,4 +47,89 @@ describe Gitlab::RepositoryCache do
       expect(backend).to have_received(:fetch).with("baz:#{namespace}", &p)
     end
   end
+
+  describe '#fetch_without_caching_false', :use_clean_rails_memory_store_caching do
+    let(:key) { :foo }
+    let(:backend) { Rails.cache }
+
+    it 'requires a block' do
+      expect do
+        cache.fetch_without_caching_false(key)
+      end.to raise_error(LocalJumpError)
+    end
+
+    context 'when the key does not exist in the cache' do
+      context 'when the result of the block is truthy' do
+        it 'returns the result of the block' do
+          result = cache.fetch_without_caching_false(key) { true }
+
+          expect(result).to be true
+        end
+
+        it 'caches the value' do
+          expect(backend).to receive(:write).with("#{key}:#{namespace}", true)
+
+          cache.fetch_without_caching_false(key) { true }
+        end
+      end
+
+      context 'when the result of the block is falsey' do
+        let(:p) { -> { false } }
+
+        it 'returns the result of the block' do
+          result = cache.fetch_without_caching_false(key, &p)
+
+          expect(result).to be false
+        end
+
+        it 'does not cache the value' do
+          expect(backend).not_to receive(:write).with("#{key}:#{namespace}", true)
+
+          cache.fetch_without_caching_false(key, &p)
+        end
+      end
+    end
+
+    context 'when the cached value is truthy' do
+      before do
+        backend.write("#{key}:#{namespace}", true)
+      end
+
+      it 'returns the cached value' do
+        result = cache.fetch_without_caching_false(key) { 'block result' }
+
+        expect(result).to be true
+      end
+
+      it 'does not execute the block' do
+        expect do |b|
+          cache.fetch_without_caching_false(key, &b)
+        end.not_to yield_control
+      end
+
+      it 'does not write to the cache' do
+        expect(backend).not_to receive(:write)
+
+        cache.fetch_without_caching_false(key) { 'block result' }
+      end
+    end
+
+    context 'when the cached value is falsey' do
+      before do
+        backend.write("#{key}:#{namespace}", false)
+      end
+
+      it 'returns the result of the block' do
+        result = cache.fetch_without_caching_false(key) { 'block result' }
+
+        expect(result).to eq 'block result'
+      end
+
+      it 'writes the truthy value to the cache' do
+        expect(backend).to receive(:write).with("#{key}:#{namespace}", 'block result')
+
+        cache.fetch_without_caching_false(key) { 'block result' }
+      end
+    end
+  end
 end
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index dffac05152b..b2869104e02 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -1044,6 +1044,47 @@ describe Repository do
         expect_to_raise_storage_error { broken_repository.exists? }
       end
     end
+
+    context 'asymmetric caching', :use_clean_rails_memory_store_caching, :request_store do
+      let(:cache) { repository.send(:cache) }
+      let(:request_store_cache) { repository.send(:request_store_cache) }
+
+      context 'when it returns true' do
+        before do
+          expect(repository.raw_repository).to receive(:exists?).once.and_return(true)
+        end
+
+        it 'caches the output in RequestStore' do
+          expect do
+            repository.exists?
+          end.to change { request_store_cache.read(:exists?) }.from(nil).to(true)
+        end
+
+        it 'caches the output in RepositoryCache' do
+          expect do
+            repository.exists?
+          end.to change { cache.read(:exists?) }.from(nil).to(true)
+        end
+      end
+
+      context 'when it returns false' do
+        before do
+          expect(repository.raw_repository).to receive(:exists?).once.and_return(false)
+        end
+
+        it 'caches the output in RequestStore' do
+          expect do
+            repository.exists?
+          end.to change { request_store_cache.read(:exists?) }.from(nil).to(false)
+        end
+
+        it 'does NOT cache the output in RepositoryCache' do
+          expect do
+            repository.exists?
+          end.not_to change { cache.read(:exists?) }.from(nil)
+        end
+      end
+    end
   end
 
   describe '#has_visible_content?' do
@@ -1892,7 +1933,7 @@ describe Repository do
         match[1].to_sym if match
       end.compact
 
-      expect(methods).to match_array(Repository::CACHED_METHODS + Repository::MEMOIZED_CACHED_METHODS)
+      expect(Repository::CACHED_METHODS + Repository::MEMOIZED_CACHED_METHODS).to include(*methods)
     end
   end