Merge branch 'tags-api-performance' into 'master'

API JSON caching for tags endpoint [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!54975

app/models/concerns/protected_ref.rb

@@ -4,7 +4,7 @@ module ProtectedRef
   extend ActiveSupport::Concern
 
   included do
-    belongs_to :project
+    belongs_to :project, touch: true
 
     validates :name, presence: true
     validates :project, presence: true

app/models/release.rb

@@ -8,7 +8,7 @@ class Release < ApplicationRecord
 
   cache_markdown_field :description
 
-  belongs_to :project
+  belongs_to :project, touch: true
   # releases prior to 11.7 have no author
   belongs_to :author, class_name: 'User'
 

changelogs/unreleased/tags-api-performance.yml

+---
+title: API JSON caching for tags endpoint
+merge_request: 54975
+author:
+type: performance

config/feature_flags/development/api_caching_tags.yml

+---
+name: api_caching_tags
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/54975
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/324391
+milestone: '13.10'
+type: development
+group: group::source code
+default_enabled: false

lib/api/helpers.rb

@@ -3,6 +3,7 @@
 module API
   module Helpers
     include Gitlab::Utils
+    include Helpers::Caching
     include Helpers::Pagination
     include Helpers::PaginationStrategies
 

lib/api/helpers/caching.rb

+# frozen_string_literal: true
+
+# Grape helpers for caching.
+#
+# This module helps introduce standardised caching into the Grape API
+# in a similar manner to the standard Grape DSL.
+
+module API
+  module Helpers
+    module Caching
+      # @return [ActiveSupport::Duration]
+      DEFAULT_EXPIRY = 1.day
+
+      # @return [ActiveSupport::Cache::Store]
+      def cache
+        Rails.cache
+      end
+
+      # This is functionally equivalent to the standard `#present` used in
+      # Grape endpoints, but the JSON for the object, or for each object of
+      # a collection, will be cached.
+      #
+      # With a collection all the keys will be fetched in a single call and the
+      # Entity rendered for those missing from the cache, which are then written
+      # back into it.
+      #
+      # Both the single object, and all objects inside a collection, must respond
+      # to `#cache_key`.
+      #
+      # To override the Grape formatter we return a custom wrapper in
+      # `Gitlab::Json::PrecompiledJson` which tells the `Gitlab::Json::GrapeFormatter`
+      # to export the string without conversion.
+      #
+      # A cache context can be supplied to add more context to the cache key. This
+      # defaults to including the `current_user` in every key for safety, unless overridden.
+      #
+      # @param obj_or_collection [Object, Enumerable<Object>] the object or objects to render
+      # @param with [Grape::Entity] the entity to use for rendering
+      # @param cache_context [Proc] a proc to call for each object to provide more context to the cache key
+      # @param expires_in [ActiveSupport::Duration, Integer] an expiry time for the cache entry
+      # @param presenter_args [Hash] keyword arguments to be passed to the entity
+      # @return [Gitlab::Json::PrecompiledJson]
+      def present_cached(obj_or_collection, with:, cache_context: -> (_) { current_user.cache_key }, expires_in: DEFAULT_EXPIRY, **presenter_args)
+        json =
+          if obj_or_collection.is_a?(Enumerable)
+            cached_collection(
+              obj_or_collection,
+              presenter: with,
+              presenter_args: presenter_args,
+              context: cache_context,
+              expires_in: expires_in
+            )
+          else
+            cached_object(
+              obj_or_collection,
+              presenter: with,
+              presenter_args: presenter_args,
+              context: cache_context,
+              expires_in: expires_in
+            )
+          end
+
+        body Gitlab::Json::PrecompiledJson.new(json)
+      end
+
+      private
+
+      # Optionally uses a `Proc` to add context to a cache key
+      #
+      # @param object [Object] must respond to #cache_key
+      # @param context [Proc] a proc that will be called with the object as an argument, and which should return a
+      #                       string or array of strings to be combined into the cache key
+      # @return [String]
+      def contextual_cache_key(object, context)
+        return object.cache_key if context.nil?
+
+        [object.cache_key, context.call(object)].flatten.join(":")
+      end
+
+      # Used for fetching or rendering a single object
+      #
+      # @param object [Object] the object to render
+      # @param presenter [Grape::Entity]
+      # @param presenter_args [Hash] keyword arguments to be passed to the entity
+      # @param context [Proc]
+      # @param expires_in [ActiveSupport::Duration, Integer] an expiry time for the cache entry
+      # @return [String]
+      def cached_object(object, presenter:, presenter_args:, context:, expires_in:)
+        cache.fetch(contextual_cache_key(object, context), expires_in: expires_in) do
+          Gitlab::Json.dump(presenter.represent(object, **presenter_args).as_json)
+        end
+      end
+
+      # Used for fetching or rendering multiple objects
+      #
+      # @param objects [Enumerable<Object>] the objects to render
+      # @param presenter [Grape::Entity]
+      # @param presenter_args [Hash] keyword arguments to be passed to the entity
+      # @param context [Proc]
+      # @param expires_in [ActiveSupport::Duration, Integer] an expiry time for the cache entry
+      # @return [Array<String>]
+      def cached_collection(collection, presenter:, presenter_args:, context:, expires_in:)
+        json = fetch_multi(collection, context: context, expires_in: expires_in) do |obj|
+          Gitlab::Json.dump(presenter.represent(obj, **presenter_args).as_json)
+        end
+
+        json.values
+      end
+
+      # An adapted version of ActiveSupport::Cache::Store#fetch_multi.
+      #
+      # The original method only provides the missing key to the block,
+      # not the missing object, so we have to create a map of cache keys
+      # to the objects to allow us to pass the object to the missing value
+      # block.
+      #
+      # The result is that this is functionally identical to `#fetch`.
+      def fetch_multi(*objs, context:, **kwargs)
+        objs.flatten!
+        map = multi_key_map(objs, context: context)
+
+        cache.fetch_multi(*map.keys, **kwargs) do |key|
+          yield map[key]
+        end
+      end
+
+      # @param objects [Enumerable<Object>] objects which _must_ respond to `#cache_key`
+      # @param context [Proc] a proc that can be called to help generate each cache key
+      # @return [Hash]
+      def multi_key_map(objects, context:)
+        objects.index_by do |object|
+          contextual_cache_key(object, context)
+        end
+      end
+    end
+  end
+end

lib/api/tags.rb

@@ -28,7 +28,13 @@ module API
                                 sort: "#{params[:order_by]}_#{params[:sort]}",
                                 search: params[:search]).execute
 
-        present paginate(::Kaminari.paginate_array(tags)), with: Entities::Tag, project: user_project
+        paginated_tags = paginate(::Kaminari.paginate_array(tags))
+
+        if Feature.enabled?(:api_caching_tags, user_project, type: :development)
+          present_cached paginated_tags, with: Entities::Tag, project: user_project, cache_context: -> (_tag) { user_project.cache_key }
+        else
+          present paginated_tags, with: Entities::Tag, project: user_project
+        end
       end
 
       desc 'Get a single repository tag' do

lib/gitlab/git/tag.rb

@@ -87,6 +87,10 @@ module Gitlab
         end
       end
 
+      def cache_key
+        "tag:" + Digest::SHA1.hexdigest([name, message, target, target_commit&.sha].join)
+      end
+
       private
 
       def message_from_gitaly_tag

lib/gitlab/json.rb

@@ -186,9 +186,14 @@ module Gitlab
       # The `env` param is ignored because it's not needed in either our formatter or Grape's,
       # but it is passed through for consistency.
       #
+      # If explicitly supplied with a `PrecompiledJson` instance it will skip conversion
+      # and return it directly. This is mostly used in caching.
+      #
       # @param object [Object]
       # @return [String]
       def self.call(object, env = nil)
+        return object.to_s if object.is_a?(PrecompiledJson)
+
         if Feature.enabled?(:grape_gitlab_json, default_enabled: true)
           Gitlab::Json.dump(object)
         else
@@ -197,6 +202,34 @@ module Gitlab
       end
     end
 
+    # Wrapper class used to skip JSON dumping on Grape endpoints.
+
+    class PrecompiledJson
+      UnsupportedFormatError = Class.new(StandardError)
+
+      # @overload PrecompiledJson.new("foo")
+      #   @param value [String]
+      #
+      # @overload PrecompiledJson.new(["foo", "bar"])
+      #   @param value [Array<String>]
+      def initialize(value)
+        @value = value
+      end
+
+      # Convert the value to a String. This will invoke
+      # `#to_s` on the members of the value if it's an array.
+      #
+      # @return [String]
+      # @raise [NoMethodError] if the objects in an array doesn't support to_s
+      # @raise [PrecompiledJson::UnsupportedFormatError] if the value is neither a String or Array
+      def to_s
+        return @value if @value.is_a?(String)
+        return "[#{@value.join(',')}]" if @value.is_a?(Array)
+
+        raise UnsupportedFormatError
+      end
+    end
+
     class LimitedEncoder
       LimitExceeded = Class.new(StandardError)
 

spec/factories_spec.rb

@@ -65,14 +65,23 @@ RSpec.describe 'factories' do
   # associations must be unique and cannot be reused, or the factory default
   # is being mutated.
   skip_factory_defaults = %i[
+    evidence
+    exported_protected_branch
     fork_network_member
     group_member
     import_state
+    milestone_release
     namespace
     project_broken_repo
     prometheus_alert
     prometheus_alert_event
     prometheus_metric
+    protected_branch
+    protected_branch_merge_access_level
+    protected_branch_push_access_level
+    protected_tag
+    release
+    release_link
     self_managed_prometheus_alert_event
     users_star_project
     wiki_page

spec/lib/api/helpers/caching_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe API::Helpers::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(:presenter) { API::Entities::Todo }
+
+    let(:kwargs) do
+      {
+        with: presenter,
+        project: project
+      }
+    end
+
+    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 }
+    end
+
+    context "single object" do
+      let_it_be(:presentable) { create(:todo, project: project) }
+
+      it { is_expected.to be_a(Gitlab::Json::PrecompiledJson) }
+
+      it "uses the presenter" do
+        expect(presenter).to receive(:represent).with(presentable, project: project)
+
+        subject
+      end
+
+      it "is valid JSON" do
+        parsed = Gitlab::Json.parse(subject.to_s)
+
+        expect(parsed).to be_a(Hash)
+        expect(parsed["id"]).to eq(presentable.id)
+      end
+
+      it "fetches from the cache" do
+        expect(instance.cache).to receive(:fetch).with("#{presentable.cache_key}:#{user.cache_key}", expires_in: described_class::DEFAULT_EXPIRY).once
+
+        subject
+      end
+
+      context "when a cache context is supplied" do
+        before do
+          kwargs[:cache_context] = -> (todo) { todo.project.cache_key }
+        end
+
+        it "uses the context to augment the cache key" do
+          expect(instance.cache).to receive(:fetch).with("#{presentable.cache_key}:#{project.cache_key}", expires_in: described_class::DEFAULT_EXPIRY).once
+
+          subject
+        end
+      end
+
+      context "when expires_in is supplied" do
+        it "sets the expiry when accessing the cache" do
+          kwargs[:expires_in] = 7.days
+
+          expect(instance.cache).to receive(:fetch).with("#{presentable.cache_key}:#{user.cache_key}", expires_in: 7.days).once
+
+          subject
+        end
+      end
+    end
+
+    context "for a collection of objects" do
+      let_it_be(:presentable) { Array.new(5).map { create(:todo, project: project) } }
+
+      it { is_expected.to be_an(Gitlab::Json::PrecompiledJson) }
+
+      it "uses the presenter" do
+        presentable.each do |todo|
+          expect(presenter).to receive(:represent).with(todo, project: project)
+        end
+
+        subject
+      end
+
+      it "is valid JSON" do
+        parsed = Gitlab::Json.parse(subject.to_s)
+
+        expect(parsed).to be_an(Array)
+
+        presentable.each_with_index do |todo, i|
+          expect(parsed[i]["id"]).to eq(todo.id)
+        end
+      end
+
+      it "fetches from the cache" do
+        keys = presentable.map { |todo| "#{todo.cache_key}:#{user.cache_key}" }
+
+        expect(instance.cache).to receive(:fetch_multi).with(*keys, expires_in: described_class::DEFAULT_EXPIRY).once.and_call_original
+
+        subject
+      end
+
+      context "when a cache context is supplied" do
+        before do
+          kwargs[:cache_context] = -> (todo) { todo.project.cache_key }
+        end
+
+        it "uses the context to augment the cache key" do
+          keys = presentable.map { |todo| "#{todo.cache_key}:#{project.cache_key}" }
+
+          expect(instance.cache).to receive(:fetch_multi).with(*keys, expires_in: described_class::DEFAULT_EXPIRY).once.and_call_original
+
+          subject
+        end
+      end
+
+      context "expires_in is supplied" do
+        it "sets the expiry when accessing the cache" do
+          keys = presentable.map { |todo| "#{todo.cache_key}:#{user.cache_key}" }
+          kwargs[:expires_in] = 7.days
+
+          expect(instance.cache).to receive(:fetch_multi).with(*keys, expires_in: 7.days).once.and_call_original
+
+          subject
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/git/tag_spec.rb

@@ -101,4 +101,17 @@ RSpec.describe Gitlab::Git::Tag, :seed_helper do
       end
     end
   end
+
+  describe "#cache_key" do
+    subject { repository.tags.first }
+
+    it "returns a cache key that changes based on changeable values" do
+      expect(subject).to receive(:name).and_return("v1.0.0")
+      expect(subject).to receive(:message).and_return("Initial release")
+
+      digest = Digest::SHA1.hexdigest(["v1.0.0", "Initial release", subject.target, subject.target_commit.sha].join)
+
+      expect(subject.cache_key).to eq("tag:#{digest}")
+    end
+  end
 end

spec/lib/gitlab/json_spec.rb

@@ -348,6 +348,66 @@ RSpec.describe Gitlab::Json do
         subject
       end
     end
+
+    context "precompiled JSON" do
+      let(:obj) { Gitlab::Json::PrecompiledJson.new(result) }
+
+      it "renders the string directly" do
+        expect(subject).to eq(result)
+      end
+
+      it "calls #to_s on the object" do
+        expect(obj).to receive(:to_s).once
+
+        subject
+      end
+
+      it "doesn't run the JSON formatter" do
+        expect(Gitlab::Json).not_to receive(:dump)
+
+        subject
+      end
+    end
+  end
+
+  describe Gitlab::Json::PrecompiledJson do
+    subject(:precompiled) { described_class.new(obj) }
+
+    describe "#to_s" do
+      subject { precompiled.to_s }
+
+      context "obj is a string" do
+        let(:obj) { "{}" }
+
+        it "returns a string" do
+          expect(subject).to eq("{}")
+        end
+      end
+
+      context "obj is an array" do
+        let(:obj) { ["{\"foo\": \"bar\"}", "{}"] }
+
+        it "returns a string" do
+          expect(subject).to eq("[{\"foo\": \"bar\"},{}]")
+        end
+      end
+
+      context "obj is an array of un-stringables" do
+        let(:obj) { [BasicObject.new] }
+
+        it "raises an error" do
+          expect { subject }.to raise_error(NoMethodError)
+        end
+      end
+
+      context "obj is something else" do
+        let(:obj) { {} }
+
+        it "raises an error" do
+          expect { subject }.to raise_error(described_class::UnsupportedFormatError)
+        end
+      end
+    end
   end
 
   describe Gitlab::Json::LimitedEncoder do

spec/models/protected_tag_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe ProtectedTag do
   describe 'Associations' do
-    it { is_expected.to belong_to(:project) }
+    it { is_expected.to belong_to(:project).touch(true) }
   end
 
   describe 'Validation' do

spec/models/release_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe Release do
   it { expect(release).to be_valid }
 
   describe 'associations' do
-    it { is_expected.to belong_to(:project) }
+    it { is_expected.to belong_to(:project).touch(true) }
     it { is_expected.to belong_to(:author).class_name('User') }
     it { is_expected.to have_many(:links).class_name('Releases::Link') }
     it { is_expected.to have_many(:milestones) }

spec/requests/api/tags_spec.rb

@@ -17,6 +17,7 @@ RSpec.describe API::Tags do
   end
 
   describe 'GET /projects/:id/repository/tags' do
+    shared_examples "get repository tags" do
       let(:route) { "/projects/#{project_id}/repository/tags" }
 
       context 'sorting' do
@@ -144,6 +145,71 @@ RSpec.describe API::Tags do
       end
     end
 
+    context ":api_caching_tags flag enabled", :use_clean_rails_memory_store_caching do
+      before do
+        stub_feature_flags(api_caching_tags: true)
+      end
+
+      it_behaves_like "get repository tags"
+
+      describe "cache expiry" do
+        let(:route) { "/projects/#{project_id}/repository/tags" }
+        let(:current_user) { user }
+
+        before do
+          # Set the cache
+          get api(route, current_user)
+        end
+
+        it "is cached" do
+          expect(API::Entities::Tag).not_to receive(:represent)
+
+          get api(route, current_user)
+        end
+
+        shared_examples "cache expired" do
+          it "isn't cached" do
+            expect(API::Entities::Tag).to receive(:represent).exactly(3).times
+
+            get api(route, current_user)
+          end
+        end
+
+        context "when protected tag is changed" do
+          before do
+            create(:protected_tag, name: tag_name, project: project)
+          end
+
+          it_behaves_like "cache expired"
+        end
+
+        context "when release is changed" do
+          before do
+            create(:release, :legacy, project: project, tag: tag_name)
+          end
+
+          it_behaves_like "cache expired"
+        end
+
+        context "when project is changed" do
+          before do
+            project.touch
+          end
+
+          it_behaves_like "cache expired"
+        end
+      end
+    end
+
+    context ":api_caching_tags flag disabled" do
+      before do
+        stub_feature_flags(api_caching_tags: false)
+      end
+
+      it_behaves_like "get repository tags"
+    end
+  end
+
   describe 'GET /projects/:id/repository/tags/:tag_name' do
     let(:route) { "/projects/#{project_id}/repository/tags/#{tag_name}" }