Merge branch '222780-batched-suggestions-get-applied-in-wrong-location' into 'master'

Fix incorrect Batched suggestions applications

See merge request gitlab-org/gitlab!34828

lib/gitlab/suggestions/file_suggestion.rb

@@ -7,17 +7,14 @@ module Gitlab
 
       SuggestionForDifferentFileError = Class.new(StandardError)
 
-      def initialize
-        @suggestions = []
-      end
-
-      def add_suggestion(new_suggestion)
-        if for_different_file?(new_suggestion)
-          raise SuggestionForDifferentFileError,
-                'Only add suggestions for the same file.'
-        end
+      attr_reader :file_path
+      attr_reader :blob
+      attr_reader :suggestions
 
-        suggestions << new_suggestion
+      def initialize(file_path, suggestions)
+        @file_path = file_path
+        @suggestions = suggestions.sort_by(&:from_line_index)
+        @blob = suggestions.first&.diff_file&.new_blob
       end
 
       def line_conflict?
@@ -30,18 +27,8 @@ module Gitlab
         @new_content ||= _new_content
       end
 
-      def file_path
-        @file_path ||= _file_path
-      end
-
       private
 
-      attr_accessor :suggestions
-
-      def blob
-        first_suggestion&.diff_file&.new_blob
-      end
-
       def blob_data_lines
         blob.load_all_data!
         blob.data.lines
@@ -53,31 +40,19 @@ module Gitlab
 
       def _new_content
         current_content.tap do |content|
+          # NOTE: We need to cater for line number changes when the range is more than one line.
+          offset = 0
+
           suggestions.each do |suggestion|
-            range = line_range(suggestion)
+            range = line_range(suggestion, offset)
             content[range] = suggestion.to_content
+            offset += range.count - 1
           end
         end.join
       end
 
-      def line_range(suggestion)
-        suggestion.from_line_index..suggestion.to_line_index
-      end
-
-      def for_different_file?(suggestion)
-        file_path && file_path != suggestion_file_path(suggestion)
-      end
-
-      def suggestion_file_path(suggestion)
-        suggestion&.diff_file&.file_path
-      end
-
-      def first_suggestion
-        suggestions.first
-      end
-
-      def _file_path
-        suggestion_file_path(first_suggestion)
+      def line_range(suggestion, offset = 0)
+        (suggestion.from_line_index - offset)..(suggestion.to_line_index - offset)
       end
 
       def _line_conflict?

lib/gitlab/suggestions/suggestion_set.rb

@@ -26,10 +26,10 @@ module Gitlab
       end
 
       def actions
-        @actions ||= suggestions_per_file.map do |file_path, file_suggestion|
+        @actions ||= suggestions_per_file.map do |file_suggestion|
           {
             action: 'update',
-            file_path: file_path,
+            file_path: file_suggestion.file_path,
             content: file_suggestion.new_content
           }
         end
@@ -50,19 +50,9 @@ module Gitlab
       end
 
       def _suggestions_per_file
-        suggestions.each_with_object({}) do |suggestion, result|
-          file_path = suggestion.diff_file.file_path
-          file_suggestion = result[file_path] ||= FileSuggestion.new
-          file_suggestion.add_suggestion(suggestion)
-        end
-      end
-
-      def file_suggestions
-        suggestions_per_file.values
-      end
-
-      def first_file_suggestion
-        file_suggestions.first
+        suggestions
+          .group_by { |suggestion| suggestion.diff_file.file_path }
+          .map { |file_path, group| FileSuggestion.new(file_path, group) }
       end
 
       def _error_message
@@ -72,7 +62,7 @@ module Gitlab
           return message if message
         end
 
-        has_line_conflict = file_suggestions.any? do |file_suggestion|
+        has_line_conflict = suggestions_per_file.any? do |file_suggestion|
           file_suggestion.line_conflict?
         end
 

spec/lib/gitlab/suggestions/file_suggestion_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe Gitlab::Suggestions::FileSuggestion do
-  def create_suggestion(new_line, to_content)
+  def create_suggestion(new_line, to_content, lines_above = 0, lines_below = 0)
     position = Gitlab::Diff::Position.new(old_path: file_path,
                                           new_path: file_path,
                                           old_line: nil,
@@ -18,6 +18,8 @@ describe Gitlab::Suggestions::FileSuggestion do
     create(:suggestion,
            :content_from_repo,
            note: diff_note,
+           lines_above: lines_above,
+           lines_below: lines_below,
            to_content: to_content)
   end
 
@@ -39,27 +41,9 @@ describe Gitlab::Suggestions::FileSuggestion do
     create_suggestion(15, "      *** SUGGESTION 2 ***\n")
   end
 
-  let(:file_suggestion) { described_class.new }
+  let(:suggestions) { [suggestion1, suggestion2] }
 
-  describe '#add_suggestion' do
-    it 'succeeds when adding a suggestion for the same file as the original' do
-      file_suggestion.add_suggestion(suggestion1)
-
-      expect { file_suggestion.add_suggestion(suggestion2) }.not_to raise_error
-    end
-
-    it 'raises an error when adding a suggestion for a different file' do
-      allow(suggestion2)
-        .to(receive_message_chain(:diff_file, :file_path)
-        .and_return('path/to/different/file'))
-
-      file_suggestion.add_suggestion(suggestion1)
-
-      expect { file_suggestion.add_suggestion(suggestion2) }.to(
-        raise_error(described_class::SuggestionForDifferentFileError)
-      )
-    end
-  end
+  let(:file_suggestion) { described_class.new(file_path, suggestions) }
 
   describe '#line_conflict' do
     def stub_suggestions(line_index_spans)
@@ -175,10 +159,10 @@ describe Gitlab::Suggestions::FileSuggestion do
   end
 
   describe '#new_content' do
-    it 'returns a blob with the suggestions applied to it' do
-      file_suggestion.add_suggestion(suggestion1)
-      file_suggestion.add_suggestion(suggestion2)
+    context 'with two suggestions' do
+      let(:suggestions) { [suggestion1, suggestion2] }
 
+      it 'returns a blob with the suggestions applied to it' do
         expected_content = <<-CONTENT.strip_heredoc
           require 'fileutils'
           require 'open3'
@@ -221,21 +205,250 @@ describe Gitlab::Suggestions::FileSuggestion do
 
         expect(file_suggestion.new_content).to eq(expected_content)
       end
+    end
+
+    context 'when no suggestions have been added' do
+      let(:suggestions) { [] }
 
-    it 'returns an empty string when no suggestions have been added' do
+      it 'returns an empty string' do
         expect(file_suggestion.new_content).to eq('')
       end
     end
 
-  describe '#file_path' do
-    it 'returns the path of the file associated with the suggestions' do
-      file_suggestion.add_suggestion(suggestion1)
+    context 'with multiline suggestions' do
+      let(:suggestions) { [multi_suggestion1, multi_suggestion2, multi_suggestion3] }
+
+      context 'when the previous suggestion increases the line count' do
+        let!(:multi_suggestion1) do
+          create_suggestion(9, "      *** SUGGESTION 1 ***\n      *** SECOND LINE ***\n      *** THIRD LINE ***\n")
+        end
+
+        let!(:multi_suggestion2) do
+          create_suggestion(15, "      *** SUGGESTION 2 ***\n      *** SECOND LINE ***\n")
+        end
+
+        let!(:multi_suggestion3) do
+          create_suggestion(19, "      chdir: *** SUGGESTION 3 ***\n")
+        end
+
+        it 'returns a blob with the suggestions applied to it' do
+          expected_content = <<-CONTENT.strip_heredoc
+          require 'fileutils'
+          require 'open3'
+
+          module Popen
+            extend self
+
+            def popen(cmd, path=nil)
+              unless cmd.is_a?(Array)
+                *** SUGGESTION 1 ***
+                *** SECOND LINE ***
+                *** THIRD LINE ***
+              end
+
+              path ||= Dir.pwd
+
+              vars = {
+                *** SUGGESTION 2 ***
+                *** SECOND LINE ***
+              }
+
+              options = {
+                chdir: *** SUGGESTION 3 ***
+              }
+
+              unless File.directory?(path)
+                FileUtils.mkdir_p(path)
+              end
+
+              @cmd_output = ""
+              @cmd_status = 0
+
+              Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
+                @cmd_output << stdout.read
+                @cmd_output << stderr.read
+                @cmd_status = wait_thr.value.exitstatus
+              end
+
+              return @cmd_output, @cmd_status
+            end
+          end
+          CONTENT
+
+          expect(file_suggestion.new_content).to eq(expected_content)
+        end
+      end
+
+      context 'when the previous suggestion decreases and increases the line count' do
+        let!(:multi_suggestion1) do
+          create_suggestion(9, "    *** SUGGESTION 1 ***\n", 1, 1)
+        end
+
+        let!(:multi_suggestion2) do
+          create_suggestion(15, "      *** SUGGESTION 2 ***\n      *** SECOND LINE ***\n")
+        end
+
+        let!(:multi_suggestion3) do
+          create_suggestion(19, "      chdir: *** SUGGESTION 3 ***\n")
+        end
+
+        it 'returns a blob with the suggestions applied to it' do
+          expected_content = <<-CONTENT.strip_heredoc
+          require 'fileutils'
+          require 'open3'
+
+          module Popen
+            extend self
+
+            def popen(cmd, path=nil)
+              *** SUGGESTION 1 ***
+
+              path ||= Dir.pwd
+
+              vars = {
+                *** SUGGESTION 2 ***
+                *** SECOND LINE ***
+              }
+
+              options = {
+                chdir: *** SUGGESTION 3 ***
+              }
+
+              unless File.directory?(path)
+                FileUtils.mkdir_p(path)
+              end
+
+              @cmd_output = ""
+              @cmd_status = 0
+
+              Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
+                @cmd_output << stdout.read
+                @cmd_output << stderr.read
+                @cmd_status = wait_thr.value.exitstatus
+              end
+
+              return @cmd_output, @cmd_status
+            end
+          end
+          CONTENT
+
+          expect(file_suggestion.new_content).to eq(expected_content)
+        end
+      end
+
+      context 'when the previous suggestion replaces with the same number of lines' do
+        let!(:multi_suggestion1) do
+          create_suggestion(9, "    *** SUGGESTION 1 ***\n    *** SECOND LINE ***\n    *** THIRD LINE ***\n", 1, 1)
+        end
+
+        let!(:multi_suggestion2) do
+          create_suggestion(15, "      *** SUGGESTION 2 ***\n")
+        end
 
-      expect(file_suggestion.file_path).to eq(file_path)
+        let!(:multi_suggestion3) do
+          create_suggestion(19, "      chdir: *** SUGGESTION 3 ***\n")
         end
 
-    it 'returns nil if no suggestions have been added' do
-      expect(file_suggestion.file_path).to be(nil)
+        it 'returns a blob with the suggestions applied to it' do
+          expected_content = <<-CONTENT.strip_heredoc
+          require 'fileutils'
+          require 'open3'
+
+          module Popen
+            extend self
+
+            def popen(cmd, path=nil)
+              *** SUGGESTION 1 ***
+              *** SECOND LINE ***
+              *** THIRD LINE ***
+
+              path ||= Dir.pwd
+
+              vars = {
+                *** SUGGESTION 2 ***
+              }
+
+              options = {
+                chdir: *** SUGGESTION 3 ***
+              }
+
+              unless File.directory?(path)
+                FileUtils.mkdir_p(path)
+              end
+
+              @cmd_output = ""
+              @cmd_status = 0
+
+              Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
+                @cmd_output << stdout.read
+                @cmd_output << stderr.read
+                @cmd_status = wait_thr.value.exitstatus
+              end
+
+              return @cmd_output, @cmd_status
+            end
+          end
+          CONTENT
+
+          expect(file_suggestion.new_content).to eq(expected_content)
+        end
+      end
+
+      context 'when the previous suggestion replaces multiple lines and the suggestions were applied out of order' do
+        let(:suggestions) { [multi_suggestion1, multi_suggestion3, multi_suggestion2] }
+
+        let!(:multi_suggestion1) do
+          create_suggestion(9, "    *** SUGGESTION 1 ***\n    *** SECOND LINE ***\n    *** THIRD LINE ***\n", 1, 1)
+        end
+
+        let!(:multi_suggestion3) do
+          create_suggestion(19, "    *** SUGGESTION 3 ***\n", 1, 1)
+        end
+
+        let!(:multi_suggestion2) do
+          create_suggestion(15, "    *** SUGGESTION 2 ***\n", 1, 1)
+        end
+
+        it 'returns a blob with the suggestions applied to it' do
+          expected_content = <<-CONTENT.strip_heredoc
+          require 'fileutils'
+          require 'open3'
+
+          module Popen
+            extend self
+
+            def popen(cmd, path=nil)
+              *** SUGGESTION 1 ***
+              *** SECOND LINE ***
+              *** THIRD LINE ***
+
+              path ||= Dir.pwd
+
+              *** SUGGESTION 2 ***
+
+              *** SUGGESTION 3 ***
+
+              unless File.directory?(path)
+                FileUtils.mkdir_p(path)
+              end
+
+              @cmd_output = ""
+              @cmd_status = 0
+
+              Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
+                @cmd_output << stdout.read
+                @cmd_output << stderr.read
+                @cmd_status = wait_thr.value.exitstatus
+              end
+
+              return @cmd_output, @cmd_status
+            end
+          end
+          CONTENT
+
+          expect(file_suggestion.new_content).to eq(expected_content)
+        end
+      end
     end
   end
 end

spec/lib/gitlab/suggestions/suggestion_set_spec.rb

@@ -87,11 +87,10 @@ describe Gitlab::Suggestions::SuggestionSet do
     it 'returns an array of hashes with proper key/value pairs' do
       first_action = suggestion_set.actions.first
 
-      file_path, file_suggestion = suggestion_set
-                                     .send(:suggestions_per_file).first
+      file_suggestion = suggestion_set.send(:suggestions_per_file).first
 
       expect(first_action[:action]).to be('update')
-      expect(first_action[:file_path]).to eq(file_path)
+      expect(first_action[:file_path]).to eq(file_suggestion.file_path)
       expect(first_action[:content]).to eq(file_suggestion.new_content)
     end
   end