Merge branch 'grape-action-caching' into 'master'

Rate-limited action caching for branches API [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!61688

config/feature_flags/development/api_caching_rate_limit_branches.yml

+---
+name: api_caching_rate_limit_branches
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61688
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/330876
+milestone: '13.12'
+type: development
+group: group::source code
+default_enabled: false

lib/api/branches.rb

@@ -38,33 +38,37 @@ module API
         optional :page_token, type: String, desc: 'Name of branch to start the paginaition from'
       end
       get ':id/repository/branches' do
-        user_project.preload_protected_branches
-
-        repository = user_project.repository
-
-        branches_finder = BranchesFinder.new(repository, declared_params(include_missing: false))
-        branches = Gitlab::Pagination::GitalyKeysetPager.new(self, user_project).paginate(branches_finder)
-
-        merged_branch_names = repository.merged_branch_names(branches.map(&:name))
-
-        if Feature.enabled?(:api_caching_branches, user_project, type: :development, default_enabled: :yaml)
-          present_cached(
-            branches,
-            with: Entities::Branch,
-            current_user: current_user,
-            project: user_project,
-            merged_branch_names: merged_branch_names,
-            expires_in: 10.minutes,
-            cache_context: -> (branch) { [current_user&.cache_key, merged_branch_names.include?(branch.name)] }
-          )
-        else
-          present(
-            branches,
-            with: Entities::Branch,
-            current_user: current_user,
-            project: user_project,
-            merged_branch_names: merged_branch_names
-          )
+        ff_enabled = Feature.enabled?(:api_caching_rate_limit_branches, user_project, default_enabled: :yaml)
+
+        cache_action_if(ff_enabled, [user_project, :branches, current_user, declared_params], expires_in: 30.seconds) do
+          user_project.preload_protected_branches
+
+          repository = user_project.repository
+
+          branches_finder = BranchesFinder.new(repository, declared_params(include_missing: false))
+          branches = Gitlab::Pagination::GitalyKeysetPager.new(self, user_project).paginate(branches_finder)
+
+          merged_branch_names = repository.merged_branch_names(branches.map(&:name))
+
+          if Feature.enabled?(:api_caching_branches, user_project, type: :development, default_enabled: :yaml)
+            present_cached(
+              branches,
+              with: Entities::Branch,
+              current_user: current_user,
+              project: user_project,
+              merged_branch_names: merged_branch_names,
+              expires_in: 10.minutes,
+              cache_context: -> (branch) { [current_user&.cache_key, merged_branch_names.include?(branch.name)] }
+            )
+          else
+            present(
+              branches,
+              with: Entities::Branch,
+              current_user: current_user,
+              project: user_project,
+              merged_branch_names: merged_branch_names
+            )
+          end
         end
       end
 

lib/api/helpers/caching.rb

@@ -11,6 +11,11 @@ module API
       # @return [ActiveSupport::Duration]
       DEFAULT_EXPIRY = 1.day
 
+      # @return [Hash]
+      DEFAULT_CACHE_OPTIONS = {
+        race_condition_ttl: 5.seconds
+      }.freeze
+
       # @return [ActiveSupport::Cache::Store]
       def cache
         Rails.cache
@@ -63,8 +68,59 @@ module API
         body Gitlab::Json::PrecompiledJson.new(json)
       end
 
+      # 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. 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
+      # @return [Gitlab::Json::PrecompiledJson]
+      def cache_action(key, **cache_opts)
+        json = cache.fetch(key, **apply_default_cache_options(cache_opts)) do
+          response = yield
+
+          if response.is_a?(Gitlab::Json::PrecompiledJson)
+            response.to_s
+          else
+            Gitlab::Json.dump(response.as_json)
+          end
+        end
+
+        body Gitlab::Json::PrecompiledJson.new(json)
+      end
+
+      # 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, **kwargs) do
+            yield
+          end
+        else
+          yield
+        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
 
+      def apply_default_cache_options(opts = {})
+        DEFAULT_CACHE_OPTIONS.merge(opts)
+      end
+
       # Optionally uses a `Proc` to add context to a cache key
       #
       # @param object [Object] must respond to #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,116 @@ 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
+      expected_kwargs = described_class::DEFAULT_CACHE_OPTIONS.merge(kwargs)
+
+      expect(expensive_thing).to receive(:do_very_expensive_action).once
+      expect(instance.cache).to receive(:fetch).with(cache_key, **expected_kwargs).exactly(5).times.and_call_original
+
+      5.times { perform }
+    end
+
+    it "handles nested cache calls" do
+      nested_call = instance.cache_action(cache_key, **kwargs) do
+        instance.cache_action([:nested], **kwargs) do
+          expensive_thing.do_very_expensive_action
+        end
+      end
+
+      expect(nested_call.to_s).to eq(subject.to_s)
+    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

spec/requests/api/branches_spec.rb

@@ -20,7 +20,7 @@ RSpec.describe API::Branches do
     stub_feature_flags(branch_list_keyset_pagination: false)
   end
 
-  describe "GET /projects/:id/repository/branches" do
+  describe "GET /projects/:id/repository/branches", :use_clean_rails_redis_caching do
     let(:route) { "/projects/#{project_id}/repository/branches" }
 
     shared_examples_for 'repository branches' do