From 6d9eb344ed2c44a50cc98ade58be365458046325 Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 13 May 2021 12:02:39 +0100 Subject: [PATCH 1/6] Rate-limited action caching for branches API --- .../api_caching_rate_limit_branches.yml | 8 +++ lib/api/branches.rb | 58 ++++++++++--------- lib/api/helpers/caching.rb | 26 +++++++++ 3 files changed, 65 insertions(+), 27 deletions(-) create mode 100644 config/feature_flags/development/api_caching_rate_limit_branches.yml 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 00000000000000..f4ff67f7f9b7a9 --- /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: +rollout_issue_url: +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 9e16b50f2b70a3..8a912ad492a826 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&.cache_key, 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 81a58fd5b57c85..76f7843bd1e7f9 100644 --- a/lib/api/helpers/caching.rb +++ b/lib/api/helpers/caching.rb @@ -63,6 +63,32 @@ 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. + # + # @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, **cache_opts) do + Gitlab::Json.dump(yield.as_json) + end + + body Gitlab::Json::PrecompiledJson.new(json) + end + + def cache_action_if(conditional, *opts) + if conditional + cache_action(*opts) do + yield + end + else + yield + end + end + private # Optionally uses a `Proc` to add context to a cache key -- GitLab From 09d74fc02458946c99e17ead2c603f9f39d09a8c Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 13 May 2021 14:10:47 +0100 Subject: [PATCH 2/6] Add test coverage for new methods --- lib/api/branches.rb | 2 +- lib/api/helpers/caching.rb | 21 +++- spec/lib/api/helpers/caching_spec.rb | 148 +++++++++++++++++++++++---- 3 files changed, 149 insertions(+), 22 deletions(-) diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 8a912ad492a826..12573968e9b579 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -40,7 +40,7 @@ class Branches < ::API::Base 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 diff --git a/lib/api/helpers/caching.rb b/lib/api/helpers/caching.rb index 76f7843bd1e7f9..b2efff7613647e 100644 --- a/lib/api/helpers/caching.rb +++ b/lib/api/helpers/caching.rb @@ -66,7 +66,10 @@ def present_cached(obj_or_collection, with:, cache_context: -> (_) { current_use # 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 @@ def cache_action(key, **cache_opts) 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 @@ def cache_action_if(conditional, *opts) 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 diff --git a/spec/lib/api/helpers/caching_spec.rb b/spec/lib/api/helpers/caching_spec.rb index a8cd061e12307f..967fa96c323279 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,104 @@ 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 -- GitLab From 3edf027ad5f2c5bd58ecce8267d3b2f52b3f19aa Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 13 May 2021 14:24:07 +0100 Subject: [PATCH 3/6] Add race_condition_ttl support Prevents multiple writes under heavy load. --- lib/api/helpers/caching.rb | 11 ++++++++++- spec/lib/api/helpers/caching_spec.rb | 4 +++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/api/helpers/caching.rb b/lib/api/helpers/caching.rb index b2efff7613647e..9f42f5f286c1c7 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 @@ -75,7 +80,7 @@ def present_cached(obj_or_collection, with:, cache_context: -> (_) { current_use # @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, **cache_opts) do + json = cache.fetch(key, **apply_default_cache_options(cache_opts)) do Gitlab::Json.dump(yield.as_json) end @@ -106,6 +111,10 @@ def cache_action_unless(conditional, *opts, **kwargs) 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 967fa96c323279..403f891575e31d 100644 --- a/spec/lib/api/helpers/caching_spec.rb +++ b/spec/lib/api/helpers/caching_spec.rb @@ -170,8 +170,10 @@ def perform 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, **kwargs).exactly(5).times.and_call_original + expect(instance.cache).to receive(:fetch).with(cache_key, **expected_kwargs).exactly(5).times.and_call_original 5.times { perform } end -- GitLab From 87807146fcb6149c2a809bcc07deb07c38ea18b7 Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 13 May 2021 14:29:55 +0100 Subject: [PATCH 4/6] Update feature flag definition --- .../development/api_caching_rate_limit_branches.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/feature_flags/development/api_caching_rate_limit_branches.yml b/config/feature_flags/development/api_caching_rate_limit_branches.yml index f4ff67f7f9b7a9..a4feb9fc8a0347 100644 --- a/config/feature_flags/development/api_caching_rate_limit_branches.yml +++ b/config/feature_flags/development/api_caching_rate_limit_branches.yml @@ -1,7 +1,7 @@ --- name: api_caching_rate_limit_branches -introduced_by_url: -rollout_issue_url: +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 -- GitLab From 68b7a7f26f938de73a3d290cf21a85faafd9de8f Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 13 May 2021 14:52:47 +0100 Subject: [PATCH 5/6] Only use declared params in cache key --- lib/api/branches.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 12573968e9b579..1ee120f982a28e 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -40,7 +40,7 @@ class Branches < ::API::Base 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, params], expires_in: 30.seconds) do + 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 -- GitLab From 555bb5a07b6fb0f4ba80df4dcce2767efa53aa43 Mon Sep 17 00:00:00 2001 From: Robert May Date: Thu, 13 May 2021 16:36:59 +0100 Subject: [PATCH 6/6] Fix for nested cache calls --- lib/api/helpers/caching.rb | 8 +++++++- spec/lib/api/helpers/caching_spec.rb | 10 ++++++++++ spec/requests/api/branches_spec.rb | 2 +- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/lib/api/helpers/caching.rb b/lib/api/helpers/caching.rb index 9f42f5f286c1c7..f24ac7302c12d9 100644 --- a/lib/api/helpers/caching.rb +++ b/lib/api/helpers/caching.rb @@ -81,7 +81,13 @@ def present_cached(obj_or_collection, with:, cache_context: -> (_) { current_use # @return [Gitlab::Json::PrecompiledJson] def cache_action(key, **cache_opts) json = cache.fetch(key, **apply_default_cache_options(cache_opts)) do - Gitlab::Json.dump(yield.as_json) + 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) diff --git a/spec/lib/api/helpers/caching_spec.rb b/spec/lib/api/helpers/caching_spec.rb index 403f891575e31d..f94c44c7382164 100644 --- a/spec/lib/api/helpers/caching_spec.rb +++ b/spec/lib/api/helpers/caching_spec.rb @@ -177,6 +177,16 @@ def perform 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 diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb index d697ad85d95cea..a38ba782c44d2e 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 -- GitLab