Merge branch...

Merge branch '332970-introduce-caching-on-projects-mergerequests-diffscontroller-diffs_batch-json' into 'master'

Introduce caching on diffs_batch.json

See merge request gitlab-org/gitlab!64760

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -3,6 +3,7 @@
 class Projects::MergeRequests::DiffsController < Projects::MergeRequests::ApplicationController
   include DiffHelper
   include RendersNotes
+  include Gitlab::Cache::Helpers
 
   before_action :commit
   before_action :define_diff_vars
@@ -40,7 +41,16 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
       pagination_data: diffs.pagination_data
     }
 
-    render json: PaginatedDiffSerializer.new(current_user: current_user).represent(diffs, options)
+    if diff_options_hash[:paths].blank? && Feature.enabled?(:diffs_batch_render_cached, project, default_enabled: :yaml)
+      render_cached(
+        diffs,
+        with: PaginatedDiffSerializer.new(current_user: current_user),
+        cache_context: -> (_) { [diff_view, params[:w], params[:expanded], params[:per_page], params[:page]] },
+        **options
+      )
+    else
+      render json: PaginatedDiffSerializer.new(current_user: current_user).represent(diffs, options)
+    end
   end
 
   def diffs_metadata

config/feature_flags/development/diffs_batch_render_cached.yml

+---
+name: diffs_batch_render_cached
+introduced_by_url: https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/1509
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/334762
+milestone: '14.1'
+type: development
+group: group::code review
+default_enabled: false

lib/gitlab/diff/file_collection/commit.rb

@@ -10,6 +10,10 @@ module Gitlab
             diff_options: diff_options,
             diff_refs: commit.diff_refs)
         end
+
+        def cache_key
+          ['commit', @diffable.id]
+        end
       end
     end
   end

lib/gitlab/diff/file_collection/compare.rb

@@ -14,6 +14,10 @@ module Gitlab
         def unfold_diff_lines(positions)
           # no-op
         end
+
+        def cache_key
+          ['compare', @diffable.head.id, @diffable.base.id]
+        end
       end
     end
   end

lib/gitlab/diff/file_collection/merge_request_diff_base.rb

@@ -6,6 +6,8 @@ module Gitlab
       class MergeRequestDiffBase < Base
         extend ::Gitlab::Utils::Override
 
+        delegate :real_size, :overflow?, :cache_key, to: :@merge_request_diff
+
         def initialize(merge_request_diff, diff_options:)
           @merge_request_diff = merge_request_diff
 
@@ -44,14 +46,6 @@ module Gitlab
           diff_stats_cache.clear
         end
 
-        def real_size
-          @merge_request_diff.real_size
-        end
-
-        def overflow?
-          @merge_request_diff.overflow?
-        end
-
         private
 
         def highlight_cache
@@ -62,7 +56,7 @@ module Gitlab
 
         def diff_stats_cache
           strong_memoize(:diff_stats_cache) do
-            Gitlab::Diff::StatsCache.new(cachable_key: @merge_request_diff.cache_key)
+            Gitlab::Diff::StatsCache.new(cachable_key: cache_key)
           end
         end
 

spec/lib/gitlab/diff/file_collection/commit_spec.rb

@@ -75,4 +75,12 @@ RSpec.describe Gitlab::Diff::FileCollection::Commit do
       ]
     end
   end
+
+  describe '#cache_key' do
+    subject(:cache_key) { described_class.new(diffable, diff_options: nil).cache_key }
+
+    it 'returns with the commit id' do
+      expect(cache_key).to eq ['commit', diffable.id]
+    end
+  end
 end

spec/lib/gitlab/diff/file_collection/compare_spec.rb

@@ -15,29 +15,20 @@ RSpec.describe Gitlab::Diff::FileCollection::Compare do
                              head_commit.id)
   end
 
-  it_behaves_like 'diff statistics' do
-    let(:collection_default_args) do
-      {
-        project: diffable.project,
-        diff_options: {},
-        diff_refs: diffable.diff_refs
-      }
-    end
+  let(:diffable) { Compare.new(raw_compare, project) }
+  let(:collection_default_args) do
+    {
+      project: diffable.project,
+      diff_options: {},
+      diff_refs: diffable.diff_refs
+    }
+  end
 
-    let(:diffable) { Compare.new(raw_compare, project) }
+  it_behaves_like 'diff statistics' do
     let(:stub_path) { '.gitignore' }
   end
 
   it_behaves_like 'sortable diff files' do
-    let(:diffable) { Compare.new(raw_compare, project) }
-    let(:collection_default_args) do
-      {
-        project: diffable.project,
-        diff_options: {},
-        diff_refs: diffable.diff_refs
-      }
-    end
-
     let(:unsorted_diff_files_paths) do
       [
         '.DS_Store',
@@ -66,4 +57,12 @@ RSpec.describe Gitlab::Diff::FileCollection::Compare do
       ]
     end
   end
+
+  describe '#cache_key' do
+    subject(:cache_key) { described_class.new(diffable, **collection_default_args).cache_key }
+
+    it 'returns with head and base' do
+      expect(cache_key).to eq ['compare', head_commit.id, start_commit.id]
+    end
+  end
 end

spec/lib/gitlab/diff/file_collection/merge_request_diff_base_spec.rb

@@ -25,4 +25,12 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBase do
       end
     end
   end
+
+  describe '#cache_key' do
+    subject(:cache_key) { described_class.new(diffable, diff_options: nil).cache_key }
+
+    it 'returns cache_key from merge_request_diff' do
+      expect(cache_key).to eq diffable.cache_key
+    end
+  end
 end

spec/requests/projects/merge_requests/diffs_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Merge Requests Diffs' do
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
+
+  before do
+    project.add_maintainer(user)
+    sign_in(user)
+  end
+
+  describe 'GET diffs_batch' do
+    let(:headers) { {} }
+
+    shared_examples_for 'serializes diffs with expected arguments' do
+      it 'serializes paginated merge request diff collection' do
+        expect_next_instance_of(PaginatedDiffSerializer) do |instance|
+          expect(instance).to receive(:represent)
+            .with(an_instance_of(collection), expected_options)
+            .and_call_original
+        end
+
+        subject
+      end
+    end
+
+    def collection_arguments(pagination_data = {})
+      {
+        environment: nil,
+        merge_request: merge_request,
+        diff_view: :inline,
+        merge_ref_head_diff: nil,
+        pagination_data: {
+          total_pages: nil
+        }.merge(pagination_data)
+      }
+    end
+
+    def go(extra_params = {})
+      params = {
+        namespace_id: project.namespace.to_param,
+        project_id: project,
+        id: merge_request.iid,
+        page: 0,
+        per_page: 20,
+        format: 'json'
+      }
+
+      get diffs_batch_namespace_project_json_merge_request_path(params.merge(extra_params)), headers: headers
+    end
+
+    context 'with caching', :use_clean_rails_memory_store_caching do
+      subject { go(page: 0, per_page: 5) }
+
+      context 'when the request has not been cached' do
+        it_behaves_like 'serializes diffs with expected arguments' do
+          let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
+          let(:expected_options) { collection_arguments(total_pages: 20) }
+        end
+      end
+
+      context 'when the request has already been cached' do
+        before do
+          go(page: 0, per_page: 5)
+        end
+
+        it 'does not serialize diffs' do
+          expect_next_instance_of(PaginatedDiffSerializer) do |instance|
+            expect(instance).not_to receive(:represent)
+          end
+
+          subject
+        end
+
+        context 'with the different pagination option' do
+          subject { go(page: 5, per_page: 5) }
+
+          it_behaves_like 'serializes diffs with expected arguments' do
+            let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
+            let(:expected_options) { collection_arguments(total_pages: 20) }
+          end
+        end
+
+        context 'with the different diff_view' do
+          subject { go(page: 0, per_page: 5, view: :parallel) }
+
+          it_behaves_like 'serializes diffs with expected arguments' do
+            let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
+            let(:expected_options) { collection_arguments(total_pages: 20).merge(diff_view: :parallel) }
+          end
+        end
+
+        context 'with the different expanded option' do
+          subject { go(page: 0, per_page: 5, expanded: true ) }
+
+          it_behaves_like 'serializes diffs with expected arguments' do
+            let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
+            let(:expected_options) { collection_arguments(total_pages: 20) }
+          end
+        end
+
+        context 'with the different ignore_whitespace_change option' do
+          subject { go(page: 0, per_page: 5, w: 1) }
+
+          it_behaves_like 'serializes diffs with expected arguments' do
+            let(:collection) { Gitlab::Diff::FileCollection::Compare }
+            let(:expected_options) { collection_arguments(total_pages: 20) }
+          end
+        end
+      end
+
+      context 'when the paths is given' do
+        subject { go(page: 0, per_page: 5, paths: %w[README CHANGELOG]) }
+
+        it 'does not use cache' do
+          expect(Rails.cache).not_to receive(:fetch).with(/cache:gitlab:PaginatedDiffSerializer/).and_call_original
+
+          subject
+        end
+      end
+    end
+  end
+end