Merge branch...

Merge branch '332968-introduce-caching-on-projects-mergerequestscontroller-show-json-for-sidebar_extras' into 'master'

Introduce caching on Projects::MergeRequestsController#show.json

See merge request gitlab-org/gitlab!65217

app/controllers/projects/merge_requests_controller.rb

@@ -11,6 +11,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   include RecordUserLastActivity
   include SourcegraphDecorator
   include DiffHelper
+  include Gitlab::Cache::Helpers
 
   skip_before_action :merge_request, only: [:index, :bulk_update, :export_csv]
   before_action :apply_diff_view_cookie!, only: [:show]
@@ -91,6 +92,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     end
   end
 
+  # rubocop:disable Metrics/AbcSize
   def show
     close_merge_request_if_no_source_project
 
@@ -127,8 +129,15 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
       format.json do
         Gitlab::PollingInterval.set_header(response, interval: 10_000)
 
+        if params[:serializer] == 'sidebar_extras' && Feature.enabled?(:merge_request_show_render_cached, @project, default_enabled: :yaml)
+          render_cached(@merge_request,
+                        with: serializer,
+                        cache_context: -> (_) { [params[:serializer], current_user&.cache_key, project.emails_disabled?, issuable.subscribed?(current_user, project)] },
+                        serializer: params[:serializer])
+        else
           render json: serializer.represent(@merge_request, serializer: params[:serializer])
         end
+      end
 
       format.patch do
         break render_404 unless @merge_request.diff_refs
@@ -143,6 +152,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
       end
     end
   end
+  # rubocop:enable Metrics/AbcSize
 
   def commits
     # Get context commits from repository

config/feature_flags/development/merge_request_show_render_cached.yml

+---
+name: merge_request_show_render_cached
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65217
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/335938
+milestone: '14.2'
+type: development
+group: group::code review
+default_enabled: false

spec/requests/projects/merge_requests_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'merge requests actions' do
+  let_it_be(:project) { create(:project, :repository) }
+
+  let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
+  let(:user) { project.owner }
+
+  before do
+    sign_in(user)
+  end
+
+  describe 'GET /:namespace/:project/-/merge_requests/:iid' do
+    describe 'as json' do
+      def send_request(extra_params = {})
+        params = {
+          namespace_id: project.namespace.to_param,
+          project_id: project,
+          id: merge_request.iid,
+          format: :json
+        }
+
+        get namespace_project_merge_request_path(params.merge(extra_params))
+      end
+
+      context 'with caching', :use_clean_rails_memory_store_caching do
+        let(:params) { {} }
+
+        context 'for sidebar_extras' do
+          let(:params) { { serializer: 'sidebar_extras' } }
+
+          shared_examples_for 'a non-cached request' do
+            it 'serializes merge request' do
+              expect_next_instance_of(MergeRequestSerializer) do |instance|
+                expect(instance).to receive(:represent)
+                  .with(an_instance_of(MergeRequest), serializer: 'sidebar_extras')
+                  .and_call_original
+              end
+
+              send_request(params)
+            end
+          end
+
+          context 'when the request has not been cached' do
+            it_behaves_like 'a non-cached request'
+          end
+
+          context 'when the request has already been cached' do
+            before do
+              send_request(params)
+            end
+
+            it 'does not serialize merge request again' do
+              expect_next_instance_of(MergeRequestSerializer) do |instance|
+                expect(instance).not_to receive(:represent)
+              end
+
+              send_request(params)
+            end
+
+            context 'when the merge request is updated' do
+              let(:user2) { create(:user) }
+
+              before do
+                project.add_maintainer(user2)
+              end
+
+              def update_service(params)
+                MergeRequests::UpdateService.new(project: project, current_user: user, params: params).execute(merge_request)
+              end
+
+              context 'when the user is different' do
+                before do
+                  sign_in(user2)
+                end
+
+                it_behaves_like 'a non-cached request'
+              end
+
+              context 'when the reviewer is changed' do
+                before do
+                  update_service(reviewer_ids: [user2.id])
+                end
+
+                it_behaves_like 'a non-cached request'
+              end
+
+              context 'when the assignee is changed' do
+                before do
+                  update_service( assignee_ids: [user2.id] )
+                end
+
+                it_behaves_like 'a non-cached request'
+              end
+
+              context 'when the time_estimate is changed' do
+                before do
+                  update_service(time_estimate: 7200)
+                end
+
+                it_behaves_like 'a non-cached request'
+              end
+
+              context 'when the spend_time is changed' do
+                before do
+                  update_service(spend_time: { duration: 7200, user_id: user.id, spent_at: Time.now, note_id: nil })
+                end
+
+                it_behaves_like 'a non-cached request'
+              end
+
+              context 'when a user leaves a note' do
+                before do
+                  # We have 1 minute ThrottledTouch to account for.
+                  # It's not ideal as it means that our participants cache could be stale for about a day if a new note is created by another person or gets a mention.
+                  travel_to(Time.current + 61) do
+                    Notes::CreateService.new(project, user2, { note: 'Looks good', noteable_type: 'MergeRequest', noteable_id: merge_request.id }).execute
+                  end
+                end
+
+                it_behaves_like 'a non-cached request'
+              end
+
+              context 'when the email setting has changed in project' do
+                before do
+                  project.namespace.update_attribute(:emails_disabled, true)
+                end
+
+                it_behaves_like 'a non-cached request'
+              end
+
+              context 'when the user changes unsubscribes' do
+                before do
+                  merge_request.set_subscription(user, false, project)
+                end
+
+                it_behaves_like 'a non-cached request'
+              end
+            end
+          end
+        end
+
+        context 'for other serializer' do
+          let(:params) { { serializer: 'basic' } }
+
+          it 'does not use cache' do
+            expect(Rails.cache).not_to receive(:fetch).with(/cache:gitlab:MergeRequestSerializer:/).and_call_original
+
+            send_request(params)
+          end
+        end
+      end
+    end
+  end
+end