Merge branch 'issue-discussions-http-cache' into 'master'

Improve HTTP caching for issue discussions

See merge request gitlab-org/gitlab!72589

app/controllers/concerns/issuable_actions.rb

@@ -159,9 +159,9 @@ module IssuableActions
     discussions = Discussion.build_collection(notes, issuable)
 
     if issuable.is_a?(MergeRequest)
-      cache_context = [current_user&.cache_key, project.team.human_max_access(current_user&.id)].join(':')
-
-      render_cached(discussions, with: discussion_serializer, cache_context: -> (_) { cache_context }, context: self)
+      render_cached(discussions, with: discussion_serializer, cache_context: -> (_) { discussion_cache_context }, context: self)
+    elsif issuable.is_a?(Issue) && Feature.enabled?(:issue_discussions_http_cache, default_enabled: :yaml)
+      render json: discussion_serializer.represent(discussions, context: self) if stale?(etag: [discussion_cache_context, discussions])
     else
       render json: discussion_serializer.represent(discussions, context: self)
     end
@@ -197,6 +197,10 @@ module IssuableActions
     current_user&.user_preference&.previous_changes&.any?
   end
 
+  def discussion_cache_context
+    [current_user&.cache_key, project.team.human_max_access(current_user&.id)].join(':')
+  end
+
   def discussion_serializer
     DiscussionSerializer.new(project: project, noteable: issuable, current_user: current_user, note_entity: ProjectNoteEntity)
   end

config/feature_flags/development/issue_discussions_http_cache.yml

+---
+name: issue_discussions_http_cache
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72589
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/343309
+milestone: '14.5'
+type: development
+group: group::project management
+default_enabled: false

spec/requests/projects/issues/discussions_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'issue discussions' do
+  describe 'GET /:namespace/:project/-/issues/:iid/discussions' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:issue) { create(:issue, project: project) }
+    let_it_be(:note_author) { create(:user) }
+    let_it_be(:notes) { create_list(:note, 5, project: project, noteable: issue, author: note_author) }
+
+    before_all do
+      project.add_maintainer(user)
+    end
+
+    def get_discussions
+      get discussions_namespace_project_issue_path(namespace_id: project.namespace, project_id: project, id: issue.iid), headers: {
+        'If-None-Match' => @etag
+      }
+
+      @etag = response.etag
+    end
+
+    before do
+      sign_in(user)
+
+      get_discussions
+    end
+
+    shared_examples 'cache miss' do
+      it 'returns 200 and serializes JSON' do
+        expect(DiscussionSerializer).to receive(:new).and_call_original
+
+        get_discussions
+
+        expect(response).to have_gitlab_http_status(:ok)
+      end
+    end
+
+    shared_examples 'cache hit' do
+      it 'returns 304 without serializing JSON' do
+        expect(DiscussionSerializer).not_to receive(:new)
+
+        get_discussions
+
+        expect(response).to have_gitlab_http_status(:not_modified)
+      end
+    end
+
+    context 'when issue_discussions_http_cache is disabled' do
+      before do
+        stub_feature_flags(issue_discussions_http_cache: false)
+      end
+
+      it_behaves_like 'cache miss'
+    end
+
+    context 'when issue_discussions_http_cache is enabled' do
+      before do
+        stub_feature_flags(issue_discussions_http_cache: true)
+      end
+
+      it_behaves_like 'cache hit'
+
+      context 'when user role changes' do
+        before do
+          project.add_guest(user)
+        end
+
+        it_behaves_like 'cache miss'
+      end
+
+      context 'when emoji is awarded to a note' do
+        before do
+          travel_to(1.minute.from_now) { create(:award_emoji, awardable: notes.first) }
+        end
+
+        it_behaves_like 'cache miss'
+      end
+
+      context 'when note author name changes' do
+        before do
+          note_author.update!(name: 'New name')
+        end
+
+        it_behaves_like 'cache miss'
+      end
+
+      context 'when note author status changes' do
+        before do
+          Users::SetStatusService.new(note_author, message: "updated status").execute
+        end
+
+        it_behaves_like 'cache miss'
+      end
+
+      context 'when note author role changes' do
+        before do
+          project.add_developer(note_author)
+        end
+
+        it_behaves_like 'cache miss'
+      end
+
+      context 'when note is added' do
+        before do
+          create(:note, project: project, noteable: issue)
+        end
+
+        it_behaves_like 'cache miss'
+      end
+
+      context 'when note is modified' do
+        before do
+          notes.first.update!(note: 'edited text')
+        end
+
+        it_behaves_like 'cache miss'
+      end
+
+      context 'when note is deleted' do
+        before do
+          notes.first.destroy!
+        end
+
+        it_behaves_like 'cache miss'
+      end
+    end
+  end
+end