Implement caching on discussions.json for merge requests

This is improve performance of discussions.json by caching.

Locally, when request hits a warm cache, it shows improvement
from 10s to 1.5s for 100+ discussions. For 100+ discussions, each
request reads `203569` bytes.

This is implemented behind `merge_request_discussion_cache` feature
flag and only works for merge requests. This is to see how it is
going to perform on production and to ensure we can turn the feature
off if there are unexpected bugs.

app/controllers/concerns/issuable_actions.rb

@@ -3,6 +3,7 @@
 module IssuableActions
   extend ActiveSupport::Concern
   include Gitlab::Utils::StrongMemoize
+  include Gitlab::Cache::Helpers
 
   included do
     before_action :authorize_destroy_issuable!, only: :destroy
@@ -129,7 +130,11 @@ module IssuableActions
 
     discussions = Discussion.build_collection(notes, issuable)
 
-    render json: discussion_serializer.represent(discussions, context: self)
+    if issuable.is_a?(MergeRequest) && Feature.enabled?(:merge_request_discussion_cache, issuable.target_project, default_enabled: :yaml)
+      render_cached(discussions, with: discussion_serializer, context: self)
+    else
+      render json: discussion_serializer.represent(discussions, context: self)
+    end
   end
   # rubocop:enable CodeReuse/ActiveRecord
 

app/models/award_emoji.rb

@@ -24,8 +24,8 @@ class AwardEmoji < ApplicationRecord
   scope :named, -> (names) { where(name: names) }
   scope :awarded_by, -> (users) { where(user: users) }
 
-  after_save :expire_etag_cache
-  after_destroy :expire_etag_cache
+  after_save :expire_cache
+  after_destroy :expire_cache
 
   class << self
     def votes_for_collection(ids, type)
@@ -60,7 +60,8 @@ class AwardEmoji < ApplicationRecord
     self.name == UPVOTE_NAME
   end
 
-  def expire_etag_cache
+  def expire_cache
+    awardable.try(:bump_updated_at)
     awardable.try(:expire_etag_cache)
   end
 end

app/models/discussion.rb

@@ -158,4 +158,17 @@ class Discussion
   def reply_attributes
     first_note.slice(:type, :noteable_type, :noteable_id, :commit_id, :discussion_id)
   end
+
+  def cache_key
+    # Need this so cache will be invalidated when note within a discussion
+    # has been deleted.
+    notes_sha = Digest::SHA1.hexdigest(notes.map(&:id).join(':'))
+
+    [
+      id,
+      notes_sha,
+      notes.max_by(&:updated_at).updated_at,
+      resolved_at
+    ].join(':')
+  end
 end

app/models/note.rb

@@ -500,6 +500,13 @@ class Note < ApplicationRecord
     refs
   end
 
+  def bump_updated_at
+    # Instead of calling touch which is throttled via ThrottledTouch concern,
+    # we bump the updated_at column directly. This also prevents executing
+    # after_commit callbacks that we don't need.
+    update_column(:updated_at, Time.current)
+  end
+
   def expire_etag_cache
     noteable&.expire_note_etag_cache
   end

config/feature_flags/development/merge_request_discussion_cache.yml

+---
+name: merge_request_discussion_cache
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/64688
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/332967
+milestone: '14.1'
+type: development
+group: group::code review
+default_enabled: false

spec/models/award_emoji_spec.rb

@@ -119,6 +119,36 @@ RSpec.describe AwardEmoji do
     end
   end
 
+  describe 'bumping updated at' do
+    let(:note) { create(:note_on_issue) }
+    let(:award_emoji) { build(:award_emoji, user: build(:user), awardable: note) }
+
+    it 'calls bump_updated_at on the note when saved' do
+      expect(note).to receive(:bump_updated_at)
+
+      award_emoji.save!
+    end
+
+    it 'calls bump_updated_at on the note when destroyed' do
+      expect(note).to receive(:bump_updated_at)
+
+      award_emoji.destroy!
+    end
+
+    context 'on another awardable' do
+      let(:issue) { create(:issue) }
+      let(:award_emoji) { build(:award_emoji, user: build(:user), awardable: issue) }
+
+      it 'does not error out when saved' do
+        expect { award_emoji.save! }.not_to raise_error
+      end
+
+      it 'does not error out when destroy' do
+        expect { award_emoji.destroy! }.not_to raise_error
+      end
+    end
+  end
+
   describe '.award_counts_for_user' do
     let(:user) { create(:user) }
 

spec/models/discussion_spec.rb

@@ -51,4 +51,22 @@ RSpec.describe Discussion do
       expect(policy).to be_a(NotePolicy)
     end
   end
+
+  describe '#cache_key' do
+    let(:notes_sha) { Digest::SHA1.hexdigest("#{first_note.id}:#{second_note.id}:#{third_note.id}") }
+
+    it 'returns the cache key with ID and latest updated note updated at' do
+      expect(subject.cache_key).to eq("#{subject.id}:#{notes_sha}:#{third_note.updated_at}:")
+    end
+
+    context 'when discussion is resolved' do
+      before do
+        subject.resolve!(first_note.author)
+      end
+
+      it 'returns the cache key with resolved at' do
+        expect(subject.cache_key).to eq("#{subject.id}:#{notes_sha}:#{third_note.updated_at}:#{subject.resolved_at}")
+      end
+    end
+  end
 end

spec/requests/projects/merge_requests_discussions_spec.rb

@@ -52,5 +52,110 @@ RSpec.describe 'merge requests discussions' do
       expect { send_request }
         .to change { Gitlab::GitalyClient.get_request_count }.by_at_most(4)
     end
+
+    context 'caching', :use_clean_rails_memory_store_caching do
+      let!(:first_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
+      let!(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note, noteable: merge_request, project: project) }
+      let!(:award_emoji) { create(:award_emoji, awardable: first_note) }
+
+      before do
+        # Make a request to cache the discussions
+        send_request
+      end
+
+      shared_examples 'cache miss' do
+        it 'does not hit a warm cache' do
+          expect_next_instance_of(DiscussionSerializer) do |serializer|
+            expect(serializer).to receive(:represent) do |arg|
+              expect(arg.notes).to contain_exactly(*changed_notes)
+            end.and_call_original
+          end
+
+          send_request
+        end
+      end
+
+      it 'gets cached on subsequent requests' do
+        expect_next_instance_of(DiscussionSerializer) do |serializer|
+          expect(serializer).not_to receive(:represent)
+        end
+
+        send_request
+      end
+
+      context 'when a note in a discussion got updated' do
+        before do
+          first_note.update!(updated_at: 1.minute.from_now)
+        end
+
+        it_behaves_like 'cache miss' do
+          let(:changed_notes) { [first_note, second_note] }
+        end
+      end
+
+      context 'when a note in a discussion got resolved' do
+        before do
+          travel_to(1.minute.from_now) do
+            first_note.resolve!(user)
+          end
+        end
+
+        it_behaves_like 'cache miss' do
+          let(:changed_notes) { [first_note, second_note] }
+        end
+      end
+
+      context 'when a note is added to a discussion' do
+        let!(:third_note) { create(:diff_note_on_merge_request, in_reply_to: first_note, noteable: merge_request, project: project) }
+
+        it_behaves_like 'cache miss' do
+          let(:changed_notes) { [first_note, second_note, third_note] }
+        end
+      end
+
+      context 'when a note is removed from a discussion' do
+        before do
+          second_note.destroy!
+        end
+
+        it_behaves_like 'cache miss' do
+          let(:changed_notes) { [first_note] }
+        end
+      end
+
+      context 'when an emoji is awarded to a note in discussion' do
+        before do
+          travel_to(1.minute.from_now) do
+            create(:award_emoji, awardable: first_note)
+          end
+        end
+
+        it_behaves_like 'cache miss' do
+          let(:changed_notes) { [first_note, second_note] }
+        end
+      end
+
+      context 'when an award emoji is removed from a note in discussion' do
+        before do
+          travel_to(1.minute.from_now) do
+            award_emoji.destroy!
+          end
+        end
+
+        it_behaves_like 'cache miss' do
+          let(:changed_notes) { [first_note, second_note] }
+        end
+      end
+
+      context 'when merge_request_discussion_cache is disabled' do
+        before do
+          stub_feature_flags(merge_request_discussion_cache: false)
+        end
+
+        it_behaves_like 'cache miss' do
+          let(:changed_notes) { [first_note, second_note] }
+        end
+      end
+    end
   end
 end