Merge branch 'security-mr-conflicts-slow-17-4' into '17-4-stable-ee'

Skip content when listing conflict files with types

See merge request https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/4513



Merged-by: GitLab Release Tools Bot <delivery-team+release-tools@gitlab.com>
Approved-by: Jarka Košanová <jarka@gitlab.com>
Co-authored-by: Patrick Bajao <ebajao@gitlab.com>

app/helpers/diff_helper.rb

@@ -266,7 +266,12 @@ def conflicts_with_types
     return unless merge_request.cannot_be_merged? && merge_request.source_branch_exists? && merge_request.target_branch_exists?
     cached_conflicts_with_types do
-      conflicts_service = MergeRequests::Conflicts::ListService.new(merge_request, allow_tree_conflicts: true) # rubocop:disable CodeReuse/ServiceClass
+      # We set skip_content to true since we don't really need the content to list the conflicts and their types
+      conflicts_service = MergeRequests::Conflicts::ListService.new( # rubocop:disable CodeReuse/ServiceClass
+        merge_request,
+        allow_tree_conflicts: true,
+        skip_content: true
+      )
       {}.tap do |h|
         conflicts_service.conflicts.files.each do |file|

app/services/merge_requests/conflicts/list_service.rb

@@ -30,7 +30,8 @@ def conflicts
         @conflicts ||=
           Gitlab::Conflict::FileCollection.new(
             merge_request,
-            allow_tree_conflicts: params[:allow_tree_conflicts]
+            allow_tree_conflicts: params[:allow_tree_conflicts],
+            skip_content: params[:skip_content]
           )
       end
     end

lib/gitlab/conflict/file_collection.rb

@@ -7,14 +7,14 @@ class FileCollection
       attr_reader :merge_request, :resolver
-      def initialize(merge_request, allow_tree_conflicts: false)
+      def initialize(merge_request, allow_tree_conflicts: false, skip_content: false)
         our_commit = merge_request.source_branch_head.raw
         their_commit = merge_request.target_branch_head.raw
         @target_repo = merge_request.target_project.repository
         @source_repo = merge_request.source_project.repository.raw
         @our_commit_id = our_commit.id
         @their_commit_id = their_commit.id
-        @resolver = Gitlab::Git::Conflict::Resolver.new(@target_repo.raw, @our_commit_id, @their_commit_id, allow_tree_conflicts: allow_tree_conflicts)
+        @resolver = Gitlab::Git::Conflict::Resolver.new(@target_repo.raw, @our_commit_id, @their_commit_id, allow_tree_conflicts: allow_tree_conflicts, skip_content: skip_content)
         @merge_request = merge_request
       end

lib/gitlab/git/conflict/resolver.rb

@@ -9,16 +9,22 @@ class Resolver
         ConflictSideMissing = Class.new(StandardError)
         ResolutionError = Class.new(StandardError)
-        def initialize(target_repository, our_commit_oid, their_commit_oid, allow_tree_conflicts: false)
+        def initialize(target_repository, our_commit_oid, their_commit_oid, allow_tree_conflicts: false, skip_content: false)
           @target_repository = target_repository
           @our_commit_oid = our_commit_oid
           @their_commit_oid = their_commit_oid
           @allow_tree_conflicts = allow_tree_conflicts
+          @skip_content = skip_content
         end
         def conflicts
           @conflicts ||= wrapped_gitaly_errors do
-            gitaly_conflicts_client(@target_repository).list_conflict_files(allow_tree_conflicts: @allow_tree_conflicts).to_a
+            gitaly_conflicts_client(@target_repository)
+              .list_conflict_files(
+                allow_tree_conflicts: @allow_tree_conflicts,
+                skip_content: @skip_content
+              )
+              .to_a
           rescue GRPC::FailedPrecondition => e
             raise Gitlab::Git::Conflict::Resolver::ConflictSideMissing, e.message
           end

spec/helpers/diff_helper_spec.rb

@@ -696,7 +696,12 @@
         .with(when_renamed: true)
         .and_return(:renamed_same_file)
-      allow_next_instance_of(MergeRequests::Conflicts::ListService, merge_request, allow_tree_conflicts: true) do |svc|
+      allow_next_instance_of(
+        MergeRequests::Conflicts::ListService,
+        merge_request,
+        allow_tree_conflicts: true,
+        skip_content: true
+      ) do |svc|
         if exception.present?
           allow(svc).to receive_message_chain(:conflicts, :files).and_raise(exception)
         else

spec/lib/gitlab/conflict/file_collection_spec.rb

@@ -4,12 +4,78 @@
 RSpec.describe Gitlab::Conflict::FileCollection do
   let(:merge_request) { create(:merge_request, source_branch: 'conflict-resolvable', target_branch: 'conflict-start') }
-  let(:file_collection) { described_class.new(merge_request) }
+  let(:allow_tree_conflicts) { false }
+  let(:skip_content) { false }
+
+  let(:file_collection) do
+    described_class.new(
+      merge_request,
+      allow_tree_conflicts: allow_tree_conflicts,
+      skip_content: skip_content
+    )
+  end
   describe '#files' do
+    let(:conflict_files) { [instance_double(Gitlab::Conflict::File, path: 'file.md')] }
+    let(:git_conflict_files) { [instance_double(Gitlab::Git::Conflict::File, path: 'file.md')] }
+    let(:resolver) { instance_double(Gitlab::Git::Conflict::Resolver, conflicts: git_conflict_files) }
+
     it 'returns an array of Conflict::Files' do
       expect(file_collection.files).to all(be_an_instance_of(Gitlab::Conflict::File))
     end
+
+    it 'returns conflict files' do
+      expect(Gitlab::Git::Conflict::Resolver)
+        .to receive(:new)
+        .with(
+          merge_request.source_project.repository.raw,
+          merge_request.source_branch_head.raw.id,
+          merge_request.target_branch_head.raw.id,
+          allow_tree_conflicts: false,
+          skip_content: false
+        )
+        .and_return(resolver)
+
+      expect(file_collection.files.map(&:path)).to eq(conflict_files.map(&:path))
+    end
+
+    context 'when allow_tree_conflicts is set to true' do
+      let(:allow_tree_conflicts) { true }
+
+      it 'returns conflict files with allow_tree_conflicts as true' do
+        expect(Gitlab::Git::Conflict::Resolver)
+          .to receive(:new)
+          .with(
+            merge_request.source_project.repository.raw,
+            merge_request.source_branch_head.raw.id,
+            merge_request.target_branch_head.raw.id,
+            allow_tree_conflicts: true,
+            skip_content: false
+          )
+          .and_return(resolver)
+
+        expect(file_collection.files.map(&:path)).to eq(conflict_files.map(&:path))
+      end
+    end
+
+    context 'when skip_content is set to true' do
+      let(:skip_content) { true }
+
+      it 'returns conflict files with skip_content as true' do
+        expect(Gitlab::Git::Conflict::Resolver)
+          .to receive(:new)
+          .with(
+            merge_request.source_project.repository.raw,
+            merge_request.source_branch_head.raw.id,
+            merge_request.target_branch_head.raw.id,
+            allow_tree_conflicts: false,
+            skip_content: true
+          )
+          .and_return(resolver)
+
+        expect(file_collection.files.map(&:path)).to eq(conflict_files.map(&:path))
+      end
+    end
   end
   describe '#cache' do

spec/lib/gitlab/git/conflict/resolver_spec.rb

@@ -7,21 +7,61 @@
   let(:our_commit_oid) { 'our-commit-oid' }
   let(:their_commit_oid) { 'their-commit-oid' }
   let(:gitaly_conflicts_client) { instance_double(Gitlab::GitalyClient::ConflictsService) }
-  subject(:resolver) { described_class.new(repository, our_commit_oid, their_commit_oid) }
+  let(:allow_tree_conflicts) { false }
+  let(:skip_content) { false }
+  subject(:resolver) do
+    described_class.new(
+      repository,
+      our_commit_oid,
+      their_commit_oid,
+      allow_tree_conflicts: allow_tree_conflicts,
+      skip_content: skip_content
+    )
+  end
   describe '#conflicts' do
+    let(:conflicts) { [double] }
+
     before do
       allow(repository).to receive(:gitaly_conflicts_client).and_return(gitaly_conflicts_client)
     end
     it 'returns list of conflicts' do
-      conflicts = [double]
-      expect(gitaly_conflicts_client).to receive(:list_conflict_files).and_return(conflicts)
+      expect(gitaly_conflicts_client)
+        .to receive(:list_conflict_files)
+        .with(allow_tree_conflicts: false, skip_content: false)
+        .and_return(conflicts)
       expect(resolver.conflicts).to eq(conflicts)
     end
+    context 'when allow_tree_conflicts is set to true' do
+      let(:allow_tree_conflicts) { true }
+
+      it 'returns list of conflicts with allow_tree_conflicts as true' do
+        expect(gitaly_conflicts_client)
+          .to receive(:list_conflict_files)
+          .with(allow_tree_conflicts: true, skip_content: false)
+          .and_return(conflicts)
+
+        expect(resolver.conflicts).to eq(conflicts)
+      end
+    end
+
+    context 'when skip_content is set to true' do
+      let(:skip_content) { true }
+
+      it 'returns list of conflicts with skip_content as true' do
+        expect(gitaly_conflicts_client)
+          .to receive(:list_conflict_files)
+          .with(allow_tree_conflicts: false, skip_content: true)
+          .and_return(conflicts)
+
+        expect(resolver.conflicts).to eq(conflicts)
+      end
+    end
+
     context 'when GRPC::FailedPrecondition is raised' do
       it 'rescues and raises Gitlab::Git::Conflict::Resolver::ConflictSideMissing' do
         expect(gitaly_conflicts_client).to receive(:list_conflict_files).and_raise(GRPC::FailedPrecondition)

spec/services/merge_requests/conflicts/list_service_spec.rb

@@ -10,10 +10,6 @@ def create_merge_request(source_branch, target_branch = 'conflict-start')
       end
     end
-    def conflicts_service(merge_request)
-      described_class.new(merge_request)
-    end
-
     it 'returns a falsey value when the MR can be merged without conflicts' do
       merge_request = create_merge_request('master')
       merge_request.mark_as_mergeable
@@ -95,4 +91,58 @@ def conflicts_service(merge_request)
       expect(conflicts_service(merge_request).can_be_resolved_in_ui?).to be_falsey
     end
   end
+
+  describe '#conflicts' do
+    let(:merge_request) { build_stubbed(:merge_request) }
+    let(:file_collection) { [instance_double(Gitlab::Conflict::FileCollection)] }
+
+    it 'returns conflict file collection' do
+      expect(Gitlab::Conflict::FileCollection)
+        .to receive(:new)
+        .with(
+          merge_request,
+          allow_tree_conflicts: nil,
+          skip_content: nil
+        )
+        .and_return(file_collection)
+
+      expect(conflicts_service(merge_request).conflicts).to eq(file_collection)
+    end
+
+    context 'when allow_tree_conflicts is set to true' do
+      it 'returns conflict file collection with allow_tree_conflicts as true' do
+        expect(Gitlab::Conflict::FileCollection)
+          .to receive(:new)
+          .with(
+            merge_request,
+            allow_tree_conflicts: true,
+            skip_content: nil
+          )
+          .and_return(file_collection)
+
+        expect(conflicts_service(merge_request, allow_tree_conflicts: true).conflicts)
+          .to eq(file_collection)
+      end
+    end
+
+    context 'when skip_content is set to true' do
+      it 'returns conflict file collection with skip_content as true' do
+        expect(Gitlab::Conflict::FileCollection)
+          .to receive(:new)
+          .with(
+            merge_request,
+            allow_tree_conflicts: nil,
+            skip_content: true
+          )
+          .and_return(file_collection)
+
+        expect(conflicts_service(merge_request, skip_content: true).conflicts)
+          .to eq(file_collection)
+      end
+    end
+  end
+
+  def conflicts_service(merge_request, params = {})
+    described_class.new(merge_request, params)
+  end
 end