diff --git a/config/feature_flags/development/api_caching_rate_limit_branches.yml b/config/feature_flags/development/api_caching_rate_limit_branches.yml new file mode 100644 index 0000000000000000000000000000000000000000..a4feb9fc8a03475991644fdac041b3956430ac53 --- /dev/null +++ b/config/feature_flags/development/api_caching_rate_limit_branches.yml @@ -0,0 +1,8 @@ +--- +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 diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 9e16b50f2b70a303057bbd1058c017e166045913..1ee120f982a28ebb88770c8e39383d9dbd46501d 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -38,33 +38,37 @@ class Branches < ::API::Base 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 diff --git a/lib/api/helpers/caching.rb b/lib/api/helpers/caching.rb index 81a58fd5b57c854dcb68f847aa7d10c3e33a1281..f24ac7302c12d97a2bbe67b0646542cf36ebc102 100644 --- a/lib/api/helpers/caching.rb +++ b/lib/api/helpers/caching.rb @@ -11,6 +11,11 @@ module Caching # @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 @@ def present_cached(obj_or_collection, with:, cache_context: -> (_) { current_use 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 diff --git a/spec/lib/api/helpers/caching_spec.rb b/spec/lib/api/helpers/caching_spec.rb index a8cd061e12307f94edd2672a96a9a1b33e209684..f94c44c73821641fa17c0aa25020f1039d5d7ca5 100644 --- a/spec/lib/api/helpers/caching_spec.rb +++ b/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 @@ 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 diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb index d697ad85d95cea7ee5c3f17a6f9d09af880c9ce7..a38ba782c44d2e24a64db261cd5910842bf2f179 100644 --- a/spec/requests/api/branches_spec.rb +++ b/spec/requests/api/branches_spec.rb @@ -20,7 +20,7 @@ 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