Merge branch 'osw-support-diff-version-arguments-for-diffs-batch' into 'master'

Support diff version and whitespace arguments for diffs batch endpoint

See merge request gitlab-org/gitlab!20939

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -5,8 +5,8 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   include RendersNotes
 
   before_action :apply_diff_view_cookie!
-  before_action :commit, except: :diffs_batch
-  before_action :define_diff_vars, except: :diffs_batch
+  before_action :commit
+  before_action :define_diff_vars
   before_action :define_diff_comment_vars, except: [:diffs_batch, :diffs_metadata]
 
   def show
@@ -20,11 +20,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   def diffs_batch
     return render_404 unless Feature.enabled?(:diffs_batch_load, @merge_request.project)
 
-    diffable = @merge_request.merge_request_diff
-
-    return render_404 unless diffable
-
-    diffs = diffable.diffs_in_batch(params[:page], params[:per_page], diff_options: diff_options)
+    diffs = @compare.diffs_in_batch(params[:page], params[:per_page], diff_options: diff_options)
     positions = @merge_request.note_positions_for_paths(diffs.diff_file_paths, current_user)
 
     diffs.unfold_diff_files(positions.unfoldable)
@@ -40,8 +36,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   end
 
   def diffs_metadata
+    diffs = @compare.diffs(diff_options)
+
     render json: DiffsMetadataSerializer.new(project: @merge_request.project)
-                   .represent(@diffs, additional_attributes)
+                   .represent(diffs, additional_attributes)
   end
 
   private
@@ -50,11 +48,13 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
     [{ source_project: :namespace }, { target_project: :namespace }]
   end
 
+  # Deprecated: https://gitlab.com/gitlab-org/gitlab/issues/37735
   def render_diffs
+    diffs = @compare.diffs(diff_options)
     @environment = @merge_request.environments_for(current_user).last
 
-    @diffs.unfold_diff_files(note_positions.unfoldable)
-    @diffs.write_cache
+    diffs.unfold_diff_files(note_positions.unfoldable)
+    diffs.write_cache
 
     request = {
       current_user: current_user,
@@ -64,15 +64,14 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
 
     options = additional_attributes.merge(diff_view: diff_view)
 
-    render json: DiffsSerializer.new(request).represent(@diffs, options)
+    render json: DiffsSerializer.new(request).represent(diffs, options)
   end
 
+  # Deprecated: https://gitlab.com/gitlab-org/gitlab/issues/37735
   def define_diff_vars
     @merge_request_diffs = @merge_request.merge_request_diffs.viewable.order_id_desc
     @compare = commit || find_merge_request_diff_compare
     return render_404 unless @compare
-
-    @diffs = @compare.diffs(diff_options)
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
@@ -85,6 +84,8 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
   # rubocop: enable CodeReuse/ActiveRecord
 
   # rubocop: disable CodeReuse/ActiveRecord
+  #
+  # Deprecated: https://gitlab.com/gitlab-org/gitlab/issues/37735
   def find_merge_request_diff_compare
     @merge_request_diff =
       if diff_id = params[:diff_id].presence
@@ -127,6 +128,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
     }
   end
 
+  # Deprecated: https://gitlab.com/gitlab-org/gitlab/issues/37735
   def define_diff_comment_vars
     @new_diff_note_attrs = {
       noteable_type: 'MergeRequest',

app/models/commit.rb

@@ -12,6 +12,7 @@ class Commit
   include StaticModel
   include Presentable
   include ::Gitlab::Utils::StrongMemoize
+  include ActsAsPaginatedDiff
   include CacheMarkdownField
 
   attr_mentionable :safe_message, pipeline: :single_line

app/models/compare.rb

@@ -4,6 +4,7 @@ require 'set'
 
 class Compare
   include Gitlab::Utils::StrongMemoize
+  include ActsAsPaginatedDiff
 
   delegate :same, :head, :base, to: :@compare
 

app/models/concerns/acts_as_paginated_diff.rb

+# frozen_string_literal: true
+
+module ActsAsPaginatedDiff
+  # Comparisons going back to the repository will need proper batch
+  # loading (https://gitlab.com/gitlab-org/gitlab/issues/32859).
+  # For now, we're returning all the diffs available with
+  # no pagination data.
+  def diffs_in_batch(_batch_page, _batch_size, diff_options:)
+    diffs(diff_options)
+  end
+end

app/models/merge_request_diff.rb

@@ -309,20 +309,25 @@ class MergeRequestDiff < ApplicationRecord
   end
 
   def diffs_in_batch(batch_page, batch_size, diff_options:)
-    Gitlab::Diff::FileCollection::MergeRequestDiffBatch.new(self,
-                                                            batch_page,
-                                                            batch_size,
-                                                            diff_options: diff_options)
+    fetching_repository_diffs(diff_options) do |comparison|
+      if comparison
+        comparison.diffs_in_batch(batch_page, batch_size, diff_options: diff_options)
+      else
+        diffs_in_batch_collection(batch_page, batch_size, diff_options: diff_options)
+      end
+    end
   end
 
   def diffs(diff_options = nil)
-    if without_files? && comparison = diff_refs&.compare_in(project)
+    fetching_repository_diffs(diff_options) do |comparison|
       # It should fetch the repository when diffs are cleaned by the system.
       # We don't keep these for storage overload purposes.
       # See https://gitlab.com/gitlab-org/gitlab-foss/issues/37639
-      comparison.diffs(diff_options)
-    else
-      diffs_collection(diff_options)
+      if comparison
+        comparison.diffs(diff_options)
+      else
+        diffs_collection(diff_options)
+      end
     end
   end
 
@@ -430,6 +435,13 @@ class MergeRequestDiff < ApplicationRecord
 
   private
 
+  def diffs_in_batch_collection(batch_page, batch_size, diff_options:)
+    Gitlab::Diff::FileCollection::MergeRequestDiffBatch.new(self,
+                                                            batch_page,
+                                                            batch_size,
+                                                            diff_options: diff_options)
+  end
+
   def encode_in_base64?(diff_text)
     (diff_text.encoding == Encoding::BINARY && !diff_text.ascii_only?) ||
       diff_text.include?("\0")
@@ -487,6 +499,25 @@ class MergeRequestDiff < ApplicationRecord
     end
   end
 
+  # Yields the block with the repository Compare object if it should
+  # fetch diffs from the repository instead DB.
+  def fetching_repository_diffs(diff_options)
+    return unless block_given?
+
+    diff_options ||= {}
+
+    # Can be read as: fetch the persisted diffs if yielded without the
+    # Compare object.
+    return yield unless without_files? || diff_options[:ignore_whitespace_change]
+    return yield unless diff_refs&.complete?
+
+    comparison = diff_refs.compare_in(repository.project)
+
+    return yield unless comparison
+
+    yield(comparison)
+  end
+
   def use_external_diff?
     return false unless has_attribute?(:external_diff)
     return false unless Gitlab.config.external_diffs.enabled

lib/gitlab/diff/file_collection/base.rb

@@ -34,6 +34,18 @@ module Gitlab
           @diff_files ||= diffs.decorate! { |diff| decorate_diff!(diff) }
         end
 
+        def diff_file_paths
+          diff_files.map(&:file_path)
+        end
+
+        def pagination_data
+          {
+            current_page: nil,
+            next_page: nil,
+            total_pages: nil
+          }
+        end
+
         # This mutates `diff_files` lines.
         def unfold_diff_files(positions)
           positions_grouped_by_path = positions.group_by { |position| position.file_path }

lib/gitlab/diff/file_collection/merge_request_diff_batch.rb

@@ -29,10 +29,6 @@ module Gitlab
           }
         end
 
-        def diff_file_paths
-          diff_files.map(&:file_path)
-        end
-
         override :diffs
         def diffs
           strong_memoize(:diffs) do

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -5,6 +5,19 @@ require 'spec_helper'
 describe Projects::MergeRequests::DiffsController do
   include ProjectForksHelper
 
+  shared_examples '404 for unexistent diffable' do
+    context 'when diffable does not exists' do
+      it 'returns 404' do
+        unexistent_diff_id = 9999
+
+        go(diff_id: unexistent_diff_id)
+
+        expect(MergeRequestDiff.find_by(id: unexistent_diff_id)).to be_nil
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+  end
+
   shared_examples 'forked project with submodules' do
     render_views
 
@@ -137,6 +150,8 @@ describe Projects::MergeRequests::DiffsController do
       get :diffs_metadata, params: params.merge(extra_params)
     end
 
+    it_behaves_like '404 for unexistent diffable'
+
     context 'when not authorized' do
       let(:another_user) { create(:user) }
 
@@ -159,14 +174,6 @@ describe Projects::MergeRequests::DiffsController do
       end
     end
 
-    context 'when diffable does not exists' do
-      it 'returns 404' do
-        go(diff_id: 9999)
-
-        expect(response).to have_gitlab_http_status(404)
-      end
-    end
-
     context 'with valid diff_id' do
       it 'returns success' do
         go(diff_id: merge_request.merge_request_diff.id)
@@ -328,17 +335,53 @@ describe Projects::MergeRequests::DiffsController do
   end
 
   describe 'GET diffs_batch' do
+    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
+
+    shared_examples_for 'successful request' do
+      it 'returns success' do
+        subject
+
+        expect(response).to have_gitlab_http_status(200)
+      end
+    end
+
+    def collection_arguments(pagination_data = {})
+      {
+        merge_request: merge_request,
+        diff_view: :inline,
+        pagination_data: {
+          current_page: nil,
+          next_page: nil,
+          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: 1,
+        per_page: 20,
         format: 'json'
       }
 
       get :diffs_batch, params: params.merge(extra_params)
     end
 
+    it_behaves_like '404 for unexistent diffable'
+
     context 'when feature is disabled' do
       before do
         stub_feature_flags(diffs_batch_load: false)
@@ -365,52 +408,60 @@ describe Projects::MergeRequests::DiffsController do
       end
     end
 
-    context 'with default params' do
-      let(:expected_options) do
-        {
-          merge_request: merge_request,
-          diff_view: :inline,
-          pagination_data: {
-            current_page: 1,
-            next_page: nil,
-            total_pages: 1
-          }
-        }
+    context 'with valid diff_id' do
+      subject { go(diff_id: merge_request.merge_request_diff.id) }
+
+      it_behaves_like 'serializes diffs with expected arguments' do
+        let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
+        let(:expected_options) { collection_arguments(current_page: 1, total_pages: 1) }
       end
 
-      it 'serializes paginated merge request diff collection' do
-        expect_next_instance_of(PaginatedDiffSerializer) do |instance|
-          expect(instance).to receive(:represent)
-            .with(an_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiffBatch), expected_options)
-            .and_call_original
-        end
+      it_behaves_like 'successful request'
+    end
 
-        go
+    context 'with commit_id param' do
+      subject { go(commit_id: merge_request.diff_head_sha) }
+
+      it_behaves_like 'serializes diffs with expected arguments' do
+        let(:collection) { Gitlab::Diff::FileCollection::Commit }
+        let(:expected_options) { collection_arguments }
       end
     end
 
-    context 'with smaller diff batch params' do
-      let(:expected_options) do
-        {
-          merge_request: merge_request,
-          diff_view: :inline,
-          pagination_data: {
-            current_page: 2,
-            next_page: 3,
-            total_pages: 4
-          }
-        }
+    context 'with diff_id and start_sha params' do
+      subject do
+        go(diff_id: merge_request.merge_request_diff.id,
+           start_sha: merge_request.merge_request_diff.start_commit_sha)
       end
 
-      it 'serializes paginated merge request diff collection' do
-        expect_next_instance_of(PaginatedDiffSerializer) do |instance|
-          expect(instance).to receive(:represent)
-            .with(an_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiffBatch), expected_options)
-            .and_call_original
-        end
+      it_behaves_like 'serializes diffs with expected arguments' do
+        let(:collection) { Gitlab::Diff::FileCollection::Compare }
+        let(:expected_options) { collection_arguments }
+      end
+
+      it_behaves_like 'successful request'
+    end
 
-        go(page: 2, per_page: 5)
+    context 'with default params' do
+      subject { go }
+
+      it_behaves_like 'serializes diffs with expected arguments' do
+        let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
+        let(:expected_options) { collection_arguments(current_page: 1, total_pages: 1) }
+      end
+
+      it_behaves_like 'successful request'
+    end
+
+    context 'with smaller diff batch params' do
+      subject { go(page: 2, per_page: 5) }
+
+      it_behaves_like 'serializes diffs with expected arguments' do
+        let(:collection) { Gitlab::Diff::FileCollection::MergeRequestDiffBatch }
+        let(:expected_options) { collection_arguments(current_page: 2, next_page: 3, total_pages: 4) }
       end
+
+      it_behaves_like 'successful request'
     end
 
     it_behaves_like 'forked project with submodules'

spec/models/merge_request_diff_spec.rb

@@ -211,6 +211,65 @@ describe MergeRequestDiff do
       end
     end
 
+    describe '#diffs_in_batch' do
+      let(:diff_options) { {} }
+
+      shared_examples_for 'fetching full diffs' do
+        it 'returns diffs from repository comparison' do
+          expect_next_instance_of(Compare) do |comparison|
+            expect(comparison).to receive(:diffs_in_batch)
+              .with(1, 10, diff_options: diff_options)
+              .and_call_original
+          end
+
+          diff_with_commits.diffs_in_batch(1, 10, diff_options: diff_options)
+        end
+
+        it 'returns a Gitlab::Diff::FileCollection::Compare with full diffs' do
+          diffs = diff_with_commits.diffs_in_batch(1, 10, diff_options: diff_options)
+
+          expect(diffs).to be_a(Gitlab::Diff::FileCollection::Compare)
+          expect(diffs.diff_files.size).to be > 10
+        end
+
+        it 'returns empty pagination data' do
+          diffs = diff_with_commits.diffs_in_batch(1, 10, diff_options: diff_options)
+
+          expect(diffs.pagination_data).to eq(current_page: nil,
+                                              next_page: nil,
+                                              total_pages: nil)
+        end
+      end
+
+      context 'when no persisted files available' do
+        before do
+          diff_with_commits.clean!
+        end
+
+        it_behaves_like 'fetching full diffs'
+      end
+
+      context 'when diff_options include ignore_whitespace_change' do
+        it_behaves_like 'fetching full diffs' do
+          let(:diff_options) do
+            { ignore_whitespace_change: true }
+          end
+        end
+      end
+
+      context 'when persisted files available' do
+        it 'returns paginated diffs' do
+          diffs = diff_with_commits.diffs_in_batch(1, 10, diff_options: {})
+
+          expect(diffs).to be_a(Gitlab::Diff::FileCollection::MergeRequestDiffBatch)
+          expect(diffs.diff_files.size).to eq(10)
+          expect(diffs.pagination_data).to eq(current_page: 1,
+                                              next_page: 2,
+                                              total_pages: 2)
+        end
+      end
+    end
+
     describe '#raw_diffs' do
       context 'when the :ignore_whitespace_change option is set' do
         it 'creates a new compare object instead of using preprocessed data' do