Add test coverage for new methods

lib/api/branches.rb

@@ -40,7 +40,7 @@ module API
       get ':id/repository/branches' do
         ff_enabled = Feature.enabled?(:api_caching_rate_limit_branches, user_project, default_enabled: :yaml)
 
-        cache_action_if(ff_enabled, [user_project, :branches, current_user&.cache_key, params], expires_in: 30.seconds) do
+        cache_action_if(ff_enabled, [user_project, :branches, current_user, params], expires_in: 30.seconds) do
           user_project.preload_protected_branches
 
           repository = user_project.repository

lib/api/helpers/caching.rb

@@ -66,7 +66,10 @@ module API
       # Action caching implementation
       #
       # This allows you to wrap an entire API endpoint call in a cache, useful
-      # for short TTL caches to effectively rate-limit an endpoint.
+      # for short TTL caches to effectively rate-limit an endpoint. The block
+      # will be converted to JSON and cached, and returns a
+      # `Gitlab::Json::PrecompiledJson` object which will be exported without
+      # secondary conversion.
       #
       # @param key [Object] any object that can be converted into a cache key
       # @param expires_in [ActiveSupport::Duration, Integer] an expiry time for the cache entry
@@ -79,9 +82,12 @@ module API
         body Gitlab::Json::PrecompiledJson.new(json)
       end
 
-      def cache_action_if(conditional, *opts)
+      # Conditionally cache an action
+      #
+      # Perform a `cache_action` only if the conditional passes
+      def cache_action_if(conditional, *opts, **kwargs)
         if conditional
-          cache_action(*opts) do
+          cache_action(*opts, **kwargs) do
             yield
           end
         else
@@ -89,6 +95,15 @@ module API
         end
       end
 
+      # Conditionally cache an action
+      #
+      # Perform a `cache_action` unless the conditional passes
+      def cache_action_unless(conditional, *opts, **kwargs)
+        cache_action_if(!conditional, *opts, **kwargs) do
+          yield
+        end
+      end
+
       private
 
       # Optionally uses a `Proc` to add context to a cache key

spec/lib/api/helpers/caching_spec.rb

@@ -2,34 +2,46 @@
 
 require "spec_helper"
 
-RSpec.describe API::Helpers::Caching do
+RSpec.describe API::Helpers::Caching, :use_clean_rails_redis_caching do
   subject(:instance) { Class.new.include(described_class).new }
 
-  describe "#present_cached" do
-    let_it_be(:project) { create(:project) }
-    let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
 
-    let(:presenter) { API::Entities::Todo }
+  let(:presenter) { API::Entities::Todo }
 
-    let(:kwargs) do
-      {
-        with: presenter,
-        project: project
-      }
+  let(:return_value) do
+    {
+      foo: "bar"
+    }
+  end
+
+  let(:kwargs) do
+    {
+      expires_in: 1.minute
+    }
+  end
+
+  before do
+    # We have to stub #body as it's a Grape method
+    # unavailable in the module by itself
+    allow(instance).to receive(:body) do |data|
+      data
     end
 
+    allow(instance).to receive(:current_user) { user }
+  end
+
+  describe "#present_cached" do
     subject do
       instance.present_cached(presentable, **kwargs)
     end
 
-    before do
-      # We have to stub #body as it's a Grape method
-      # unavailable in the module by itself
-      expect(instance).to receive(:body) do |data|
-        data
-      end
-
-      allow(instance).to receive(:current_user) { user }
+    let(:kwargs) do
+      {
+        with: presenter,
+        project: project
+      }
     end
 
     context "single object" do
@@ -136,4 +148,104 @@ RSpec.describe API::Helpers::Caching do
       end
     end
   end
+
+  describe "#cache_action" do
+    def perform
+      instance.cache_action(cache_key, **kwargs) do
+        expensive_thing.do_very_expensive_action
+      end
+    end
+
+    subject { perform }
+
+    let(:expensive_thing) { double(do_very_expensive_action: return_value) }
+    let(:cache_key) do
+      [user, :foo]
+    end
+
+    it { is_expected.to be_a(Gitlab::Json::PrecompiledJson) }
+
+    it "represents the correct data" do
+      expect(subject.to_s).to eq(Gitlab::Json.dump(return_value).to_s)
+    end
+
+    it "only calls the expensive action once" do
+      expect(expensive_thing).to receive(:do_very_expensive_action).once
+      expect(instance.cache).to receive(:fetch).with(cache_key, **kwargs).exactly(5).times.and_call_original
+
+      5.times { perform }
+    end
+  end
+
+  describe "#cache_action_if" do
+    subject do
+      instance.cache_action_if(conditional, cache_key, **kwargs) do
+        return_value
+      end
+    end
+
+    let(:cache_key) do
+      [user, :conditional_if]
+    end
+
+    context "conditional is truthy" do
+      let(:conditional) { "truthy thing" }
+
+      it { is_expected.to be_a(Gitlab::Json::PrecompiledJson) }
+
+      it "caches the block" do
+        expect(instance).to receive(:cache_action).with(cache_key, **kwargs)
+
+        subject
+      end
+    end
+
+    context "conditional is falsey" do
+      let(:conditional) { false }
+
+      it { is_expected.to eq(return_value) }
+
+      it "doesn't cache the block" do
+        expect(instance).not_to receive(:cache_action).with(cache_key, **kwargs)
+
+        subject
+      end
+    end
+  end
+
+  describe "#cache_action_unless" do
+    subject do
+      instance.cache_action_unless(conditional, cache_key, **kwargs) do
+        return_value
+      end
+    end
+
+    let(:cache_key) do
+      [user, :conditional_unless]
+    end
+
+    context "conditional is truthy" do
+      let(:conditional) { "truthy thing" }
+
+      it { is_expected.to eq(return_value) }
+
+      it "doesn't cache the block" do
+        expect(instance).not_to receive(:cache_action).with(cache_key, **kwargs)
+
+        subject
+      end
+    end
+
+    context "conditional is falsey" do
+      let(:conditional) { false }
+
+      it { is_expected.to be_a(Gitlab::Json::PrecompiledJson) }
+
+      it "caches the block" do
+        expect(instance).to receive(:cache_action).with(cache_key, **kwargs)
+
+        subject
+      end
+    end
+  end
 end