Merge branch '332967-discussions-json-cache' into 'master'

Implement caching on discussions.json for merge requests

See merge request gitlab-org/gitlab!64688

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

@@ -7,6 +7,9 @@ class Discussion
   include GlobalID::Identification
   include ResolvableDiscussion
 
+  # Bump this if we need to refresh the cached versions of discussions
+  CACHE_VERSION = 1
+
   attr_reader :notes, :context_noteable
 
   delegate  :created_at,
@@ -158,4 +161,19 @@ 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(':'))
+
+    [
+      CACHE_VERSION,
+      notes.last.latest_cached_markdown_version,
+      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("#{described_class::CACHE_VERSION}:#{third_note.latest_cached_markdown_version}:#{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("#{described_class::CACHE_VERSION}:#{third_note.latest_cached_markdown_version}:#{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,121 @@ 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 cached markdown version gets bump' do
+        before do
+          settings = Gitlab::CurrentSettings.current_application_settings
+          settings.update!(local_markdown_version: settings.local_markdown_version + 1)
+        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