Merge branch '17580-enable-etag-caching-notes-for-mrs' into 'master'

Enable ETag caching for merge request notes polling

See merge request gitlab-org/gitlab!20440

app/models/commit.rb

@@ -464,8 +464,20 @@ class Commit
     "commit:#{sha}"
   end
 
+  def expire_note_etag_cache
+    super
+
+    expire_note_etag_cache_for_related_mrs
+  end
+
   private
 
+  def expire_note_etag_cache_for_related_mrs
+    MergeRequest.includes(target_project: :namespace).by_commit_sha(id).find_each do |mr|
+      mr.expire_note_etag_cache
+    end
+  end
+
   def commit_reference(from, referable_commit_id, full: false)
     reference = project.to_reference(from, full: full)
 

app/models/merge_request.rb

@@ -1468,6 +1468,10 @@ class MergeRequest < ApplicationRecord
     all_pipelines.for_sha_or_source_sha(diff_head_sha).first
   end
 
+  def etag_caching_enabled?
+    true
+  end
+
   private
 
   def with_rebase_lock

changelogs/unreleased/17580-enable-etag-caching-notes-for-mrs.yml

+---
+title: Enable ETag caching for MR notes polling
+merge_request: 20440
+author:
+type: performance

lib/gitlab/etag_caching/router.rb

@@ -22,6 +22,10 @@ module Gitlab
           %r(#{RESERVED_WORDS_PREFIX}/noteable/issue/\d+/notes\z),
           'issue_notes'
         ),
+        Gitlab::EtagCaching::Router::Route.new(
+          %r(#{RESERVED_WORDS_PREFIX}/noteable/merge_request/\d+/notes\z),
+          'merge_request_notes'
+        ),
         Gitlab::EtagCaching::Router::Route.new(
           %r(#{RESERVED_WORDS_PREFIX}/issues/\d+/realtime_changes\z),
           'issue_title'

spec/lib/gitlab/etag_caching/router_spec.rb

@@ -12,6 +12,15 @@ describe Gitlab::EtagCaching::Router do
     expect(result.name).to eq 'issue_notes'
   end
 
+  it 'matches MR notes endpoint' do
+    result = described_class.match(
+      '/my-group/and-subgroup/here-comes-the-project/noteable/merge_request/1/notes'
+    )
+
+    expect(result).to be_present
+    expect(result.name).to eq 'merge_request_notes'
+  end
+
   it 'matches issue title endpoint' do
     result = described_class.match(
       '/my-group/my-project/issues/123/realtime_changes'

spec/models/note_spec.rb

@@ -1048,20 +1048,20 @@ describe Note do
   describe 'expiring ETag cache' do
     let(:note) { build(:note_on_issue) }
 
-    def expect_expiration(note)
+    def expect_expiration(noteable)
       expect_any_instance_of(Gitlab::EtagCaching::Store)
         .to receive(:touch)
-        .with("/#{note.project.namespace.to_param}/#{note.project.to_param}/noteable/issue/#{note.noteable.id}/notes")
+        .with("/#{noteable.project.namespace.to_param}/#{noteable.project.to_param}/noteable/#{noteable.class.name.underscore}/#{noteable.id}/notes")
     end
 
     it "expires cache for note's issue when note is saved" do
-      expect_expiration(note)
+      expect_expiration(note.noteable)
 
       note.save!
     end
 
     it "expires cache for note's issue when note is destroyed" do
-      expect_expiration(note)
+      expect_expiration(note.noteable)
 
       note.destroy!
     end
@@ -1076,30 +1076,56 @@ describe Note do
       end
     end
 
+    context 'for merge requests' do
+      let_it_be(:merge_request) { create(:merge_request) }
+
+      context 'when adding a note to the MR' do
+        let(:note) { build(:note, noteable: merge_request, project: merge_request.project) }
+
+        it 'expires the MR note etag cache' do
+          expect_expiration(merge_request)
+
+          note.save!
+        end
+      end
+
+      context 'when adding a note to a commit on the MR' do
+        let(:note) { build(:note_on_commit, commit_id: merge_request.commits.first.id, project: merge_request.project) }
+
+        it 'expires the MR note etag cache' do
+          expect_expiration(merge_request)
+
+          note.save!
+        end
+      end
+    end
+  end
+
   describe '#with_notes_filter' do
     let!(:comment) { create(:note) }
     let!(:system_note) { create(:note, system: true) }
 
+    subject { described_class.with_notes_filter(filter) }
+
     context 'when notes filter is nil' do
-        subject { described_class.with_notes_filter(nil) }
+      let(:filter) { nil }
 
       it { is_expected.to include(comment, system_note) }
     end
 
     context 'when notes filter is set to all notes' do
-        subject { described_class.with_notes_filter(UserPreference::NOTES_FILTERS[:all_notes]) }
+      let(:filter) { UserPreference::NOTES_FILTERS[:all_notes] }
 
       it { is_expected.to include(comment, system_note) }
     end
 
     context 'when notes filter is set to only comments' do
-        subject { described_class.with_notes_filter(UserPreference::NOTES_FILTERS[:only_comments]) }
+      let(:filter) { UserPreference::NOTES_FILTERS[:only_comments] }
 
       it { is_expected.to include(comment) }
       it { is_expected.not_to include(system_note) }
     end
   end
-  end
 
   describe '#special_role=' do
     let(:role) { Note::SpecialRole::FIRST_TIME_CONTRIBUTOR }