Merge branch 'kassio/github-importer-replace-legacy-diff-notes-by-diff-notes' into 'master'

GithubImport: Enable users to import _modern_ diff notes

See merge request gitlab-org/gitlab!71765

app/services/import/github/notes/create_service.rb

+# frozen_string_literal: true
+
+module Import
+  module Github
+    module Notes
+      class CreateService < ::Notes::CreateService
+        # Github does not have support to quick actions in notes (like /assign)
+        # Therefore, when importing notes we skip the quick actions processing
+        def quick_actions_supported?(_note)
+          false
+        end
+      end
+    end
+  end
+end

app/services/notes/create_service.rb

@@ -43,7 +43,7 @@ module Notes
     private
 
     def execute_quick_actions(note)
-      return yield(false) unless quick_actions_service.supported?(note)
+      return yield(false) unless quick_actions_supported?(note)
 
       content, update_params, message = quick_actions_service.execute(note, quick_action_options)
       only_commands = content.empty?
@@ -54,6 +54,10 @@ module Notes
       do_commands(note, update_params, message, only_commands)
     end
 
+    def quick_actions_supported?(note)
+      quick_actions_service.supported?(note)
+    end
+
     def quick_actions_service
       @quick_actions_service ||= QuickActionsService.new(project, current_user)
     end

config/feature_flags/development/github_importer_use_diff_note_with_suggestions.yml

+---
+name: github_importer_use_diff_note_with_suggestions
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71765
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/344309
+milestone: '14.5'
+type: development
+group: group::import
+default_enabled: false

lib/gitlab/github_import/importer/diff_note_importer.rb

@@ -4,58 +4,132 @@ module Gitlab
   module GithubImport
     module Importer
       class DiffNoteImporter
-        attr_reader :note, :project, :client, :user_finder
-
-        # note - An instance of `Gitlab::GithubImport::Representation::DiffNote`.
-        # project - An instance of `Project`.
-        # client - An instance of `Gitlab::GithubImport::Client`.
+        # note - An instance of `Gitlab::GithubImport::Representation::DiffNote`
+        # project - An instance of `Project`
+        # client - An instance of `Gitlab::GithubImport::Client`
         def initialize(note, project, client)
           @note = note
           @project = project
           @client = client
-          @user_finder = GithubImport::UserFinder.new(project, client)
         end
 
         def execute
-          return unless (mr_id = find_merge_request_id)
+          return if merge_request_id.blank?
 
-          author_id, author_found = user_finder.author_id_for(note)
+          build_author_attributes
 
-          note_body = MarkdownText.format(note.note, note.author, author_found)
+          # Diff notes with suggestions are imported with DiffNote, which is
+          # slower to import than LegacyDiffNote. Importing DiffNote is slower
+          # because it cannot use the BulkImporting strategy, which skips
+          # callbacks and validations. For this reason, notes that don't have
+          # suggestions are still imported with LegacyDiffNote
+          if import_with_diff_note?
+            import_with_diff_note
+          else
+            import_with_legacy_diff_note
+          end
+        rescue ActiveRecord::InvalidForeignKey => e
+          # It's possible the project and the issue have been deleted since
+          # scheduling this job. In this case we'll just skip creating the note
+          Logger.info(
+            message: e.message,
+            github_identifiers: note.github_identifiers
+          )
+        end
 
-          attributes = {
-            discussion_id: Discussion.discussion_id(note),
+        private
+
+        attr_reader :note, :project, :client, :author_id, :author_found
+
+        def import_with_diff_note?
+          note.contains_suggestion? && use_diff_note_with_suggestions_enabled?
+        end
+
+        def use_diff_note_with_suggestions_enabled?
+          Feature.enabled?(
+            :github_importer_use_diff_note_with_suggestions,
+            default_enabled: :yaml
+          )
+        end
+
+        def build_author_attributes
+          @author_id, @author_found = user_finder.author_id_for(note)
+        end
+
+        # rubocop:disable Gitlab/BulkInsert
+        def import_with_legacy_diff_note
+          log_diff_note_creation('LegacyDiffNote')
+          # It's possible that during an import we'll insert tens of thousands
+          # of diff notes. If we were to use the Note/LegacyDiffNote model here
+          # we'd also have to run additional queries for both validations and
+          # callbacks, putting a lot of pressure on the database.
+          #
+          # To work around this we're using bulk_insert with a single row. This
+          # allows us to efficiently insert data (even if it's just 1 row)
+          # without having to use all sorts of hacks to disable callbacks.
+          Gitlab::Database.main.bulk_insert(LegacyDiffNote.table_name, [{
             noteable_type: 'MergeRequest',
-            noteable_id: mr_id,
+            system: false,
+            type: 'LegacyDiffNote',
+            discussion_id: Discussion.discussion_id(note),
+            noteable_id: merge_request_id,
             project_id: project.id,
             author_id: author_id,
             note: note_body,
-            system: false,
             commit_id: note.original_commit_id,
             line_code: note.line_code,
-            type: 'LegacyDiffNote',
             created_at: note.created_at,
             updated_at: note.updated_at,
             st_diff: note.diff_hash.to_yaml
-          }
+          }])
+        end
+        # rubocop:enabled Gitlab/BulkInsert
 
-          # It's possible that during an import we'll insert tens of thousands
-          # of diff notes. If we were to use the Note/LegacyDiffNote model here
-          # we'd also have to run additional queries for both validations and
-          # callbacks, putting a lot of pressure on the database.
-          #
-          # To work around this we're using bulk_insert with a single row. This
-          # allows us to efficiently insert data (even if it's just 1 row)
-          # without having to use all sorts of hacks to disable callbacks.
-          Gitlab::Database.main.bulk_insert(LegacyDiffNote.table_name, [attributes]) # rubocop:disable Gitlab/BulkInsert
-        rescue ActiveRecord::InvalidForeignKey
-          # It's possible the project and the issue have been deleted since
-          # scheduling this job. In this case we'll just skip creating the note.
+        def import_with_diff_note
+          log_diff_note_creation('DiffNote')
+
+          ::Import::Github::Notes::CreateService.new(project, author, {
+            noteable_type: 'MergeRequest',
+            system: false,
+            type: 'DiffNote',
+            noteable_id: merge_request_id,
+            project_id: project.id,
+            note: note_body,
+            commit_id: note.original_commit_id,
+            created_at: note.created_at,
+            updated_at: note.updated_at,
+            position: note.diff_position(merge_request)
+          }).execute
+        end
+
+        def note_body
+          @note_body ||= MarkdownText.format(note.note, note.author, author_found)
+        end
+
+        def author
+          @author ||= User.find(author_id)
+        end
+
+        def merge_request
+          @merge_request ||= MergeRequest.find(merge_request_id)
         end
 
         # Returns the ID of the merge request this note belongs to.
-        def find_merge_request_id
-          GithubImport::IssuableFinder.new(project, note).database_id
+        def merge_request_id
+          @merge_request_id ||= GithubImport::IssuableFinder.new(project, note).database_id
+        end
+
+        def user_finder
+          @user_finder ||= GithubImport::UserFinder.new(project, client)
+        end
+
+        def log_diff_note_creation(model)
+          Logger.info(
+            project_id: project.id,
+            importer: self.class.name,
+            github_identifiers: note.github_identifiers,
+            model: model
+          )
         end
       end
     end

lib/gitlab/github_import/representation/diff_note.rb

@@ -10,8 +10,9 @@ module Gitlab
         attr_reader :attributes
 
         expose_attribute :noteable_type, :noteable_id, :commit_id, :file_path,
-                         :diff_hunk, :author, :note, :created_at, :updated_at,
-                         :original_commit_id, :note_id
+                         :diff_hunk, :author, :created_at, :updated_at,
+                         :original_commit_id, :note_id, :end_line, :start_line,
+                         :side
 
         NOTEABLE_ID_REGEX = %r{/pull/(?<iid>\d+)}i.freeze
 
@@ -42,7 +43,8 @@ module Gitlab
             updated_at: note.updated_at,
             note_id: note.id,
             end_line: note.line,
-            start_line: note.start_line
+            start_line: note.start_line,
+            side: note.side
           }
 
           new(hash)
@@ -60,17 +62,31 @@ module Gitlab
         #              Hash must be Symbols.
         def initialize(attributes)
           @attributes = attributes
+
+          @note_formatter = DiffNotes::SuggestionFormatter.new(
+            note: attributes[:note],
+            start_line: attributes[:start_line],
+            end_line: attributes[:end_line]
+          )
+        end
+
+        def contains_suggestion?
+          @note_formatter.contains_suggestion?
+        end
+
+        def note
+          @note_formatter.formatted_note
         end
 
         def line_code
           diff_line = Gitlab::Diff::Parser.new.parse(diff_hunk.lines).to_a.last
 
-          Gitlab::Git
-            .diff_line_code(file_path, diff_line.new_pos, diff_line.old_pos)
+          Gitlab::Git.diff_line_code(file_path, diff_line.new_pos, diff_line.old_pos)
         end
 
         # Returns a Hash that can be used to populate `notes.st_diff`, removing
         # the need for requesting Git data for every diff note.
+        # Used when importing with LegacyDiffNote
         def diff_hash
           {
             diff: diff_hunk,
@@ -85,12 +101,15 @@ module Gitlab
           }
         end
 
-        def note
-          @note ||= DiffNotes::SuggestionFormatter.formatted_note_for(
-            note: attributes[:note],
-            start_line: attributes[:start_line],
-            end_line: attributes[:end_line]
-          )
+        # Used when importing with DiffNote
+        def diff_position(merge_request)
+          position_params = {
+            diff_refs: merge_request.diff_refs,
+            old_path: file_path,
+            new_path: file_path
+          }
+
+          Gitlab::Diff::Position.new(position_params.merge(diff_line_params))
         end
 
         def github_identifiers
@@ -100,6 +119,20 @@ module Gitlab
             noteable_type: noteable_type
           }
         end
+
+        private
+
+        def diff_line_params
+          if addition?
+            { new_line: end_line, old_line: nil }
+          else
+            { new_line: nil, old_line: end_line }
+          end
+        end
+
+        def addition?
+          side == 'RIGHT'
+        end
       end
     end
   end

lib/gitlab/github_import/representation/diff_notes/suggestion_formatter.rb

@@ -10,23 +10,25 @@ module Gitlab
     module Representation
       module DiffNotes
         class SuggestionFormatter
+          include Gitlab::Utils::StrongMemoize
+
           # A github suggestion:
           # - the ```suggestion tag must be the first text of the line
           #   - it might have up to 3 spaces before the ```suggestion tag
           # - extra text on the ```suggestion tag line will be ignored
           GITHUB_SUGGESTION = /^\ {,3}(?<suggestion>```suggestion\b).*(?<eol>\R)/.freeze
 
-          def self.formatted_note_for(...)
-            new(...).formatted_note
-          end
-
           def initialize(note:, start_line: nil, end_line: nil)
             @note = note
             @start_line = start_line
             @end_line = end_line
           end
 
+          # Returns a tuple with:
+          #   - a boolean indicating if the note has suggestions
+          #   - the note with the suggestion formatted for Gitlab
           def formatted_note
+            @formatted_note ||=
               if contains_suggestion?
                 note.gsub(
                   GITHUB_SUGGESTION,
@@ -37,13 +39,15 @@ module Gitlab
               end
           end
 
-          private
-
-          attr_reader :note, :start_line, :end_line
-
           def contains_suggestion?
+            strong_memoize(:contain_suggestion) do
               note.to_s.match?(GITHUB_SUGGESTION)
             end
+          end
+
+          private
+
+          attr_reader :note, :start_line, :end_line
 
           # Github always saves the comment on the _last_ line of the range.
           # Therefore, the diff hunk will always be related to lines before

spec/lib/gitlab/github_import/importer/diff_notes_importer_spec.rb

@@ -20,6 +20,7 @@ RSpec.describe Gitlab::GithubImport::Importer::DiffNotesImporter do
       line: 23,
       start_line: nil,
       id: 1,
+      side: 'RIGHT',
       body: <<~BODY
         Hello World
 

spec/lib/gitlab/github_import/representation/diff_notes/suggestion_formatter_spec.rb

@@ -9,13 +9,19 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat
     ```
     BODY
 
-    expect(described_class.formatted_note_for(note: note)).to eq(note)
+    note_formatter = described_class.new(note: note)
+
+    expect(note_formatter.formatted_note).to eq(note)
+    expect(note_formatter.contains_suggestion?).to eq(false)
   end
 
   it 'handles nil value for note' do
     note = nil
 
-    expect(described_class.formatted_note_for(note: note)).to eq(note)
+    note_formatter = described_class.new(note: note)
+
+    expect(note_formatter.formatted_note).to eq(note)
+    expect(note_formatter.contains_suggestion?).to eq(false)
   end
 
   it 'does not allow over 3 leading spaces for valid suggestion' do
@@ -26,7 +32,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat
       ```
     BODY
 
-    expect(described_class.formatted_note_for(note: note)).to eq(note)
+    note_formatter = described_class.new(note: note)
+
+    expect(note_formatter.formatted_note).to eq(note)
+    expect(note_formatter.contains_suggestion?).to eq(false)
   end
 
   it 'allows up to 3 leading spaces' do
@@ -44,7 +53,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat
       ```
     BODY
 
-    expect(described_class.formatted_note_for(note: note)).to eq(expected)
+    note_formatter = described_class.new(note: note)
+
+    expect(note_formatter.formatted_note).to eq(expected)
+    expect(note_formatter.contains_suggestion?).to eq(true)
   end
 
   it 'does nothing when there is any text without space after the suggestion tag' do
@@ -53,7 +65,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat
     ```
     BODY
 
-    expect(described_class.formatted_note_for(note: note)).to eq(note)
+    note_formatter = described_class.new(note: note)
+
+    expect(note_formatter.formatted_note).to eq(note)
+    expect(note_formatter.contains_suggestion?).to eq(false)
   end
 
   it 'formats single-line suggestions' do
@@ -71,7 +86,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat
       ```
     BODY
 
-    expect(described_class.formatted_note_for(note: note)).to eq(expected)
+    note_formatter = described_class.new(note: note)
+
+    expect(note_formatter.formatted_note).to eq(expected)
+    expect(note_formatter.contains_suggestion?).to eq(true)
   end
 
   it 'ignores text after suggestion tag on the same line' do
@@ -89,7 +107,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat
     ```
     BODY
 
-    expect(described_class.formatted_note_for(note: note)).to eq(expected)
+    note_formatter = described_class.new(note: note)
+
+    expect(note_formatter.formatted_note).to eq(expected)
+    expect(note_formatter.contains_suggestion?).to eq(true)
   end
 
   it 'formats multiple single-line suggestions' do
@@ -115,7 +136,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat
       ```
     BODY
 
-    expect(described_class.formatted_note_for(note: note)).to eq(expected)
+    note_formatter = described_class.new(note: note)
+
+    expect(note_formatter.formatted_note).to eq(expected)
+    expect(note_formatter.contains_suggestion?).to eq(true)
   end
 
   it 'formats multi-line suggestions' do
@@ -133,7 +157,10 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat
       ```
     BODY
 
-    expect(described_class.formatted_note_for(note: note, start_line: 6, end_line: 8)).to eq(expected)
+    note_formatter = described_class.new(note: note, start_line: 6, end_line: 8)
+
+    expect(note_formatter.formatted_note).to eq(expected)
+    expect(note_formatter.contains_suggestion?).to eq(true)
   end
 
   it 'formats multiple multi-line suggestions' do
@@ -159,6 +186,9 @@ RSpec.describe Gitlab::GithubImport::Representation::DiffNotes::SuggestionFormat
       ```
     BODY
 
-    expect(described_class.formatted_note_for(note: note, start_line: 6, end_line: 8)).to eq(expected)
+    note_formatter = described_class.new(note: note, start_line: 6, end_line: 8)
+
+    expect(note_formatter.formatted_note).to eq(expected)
+    expect(note_formatter.contains_suggestion?).to eq(true)
   end
 end

spec/services/import/github/notes/create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Import::Github::Notes::CreateService do
+  it 'does not support quick actions' do
+    project = create(:project, :repository)
+    user = create(:user)
+    merge_request = create(:merge_request, source_project: project)
+
+    project.add_maintainer(user)
+
+    note = described_class.new(
+      project,
+      user,
+      note: '/close',
+      noteable_type: 'MergeRequest',
+      noteable_id: merge_request.id
+    ).execute
+
+    expect(note.note).to eq('/close')
+    expect(note.noteable.closed?).to be(false)
+  end
+end