Merge branch 'rodrigo/add-log-to-note-importer' into 'master'

Remove InvalidForeignKey rescue and add logs in GitHub Import

See merge request !133152



Merged-by: Rodrigo Tomonari <rtomonari@gitlab.com>
Reviewed-by: Luke Duncalfe <lduncalfe@gitlab.com>
Reviewed-by: Jessie Young <jessieyoung@gitlab.com>

lib/gitlab/github_import/exceptions.rb

@@ -6,6 +6,8 @@ module Exceptions
       # Sometimes it's not clear which of not implemented interfaces  caused this error.
       # We need custom exception to be able to add text that gives extra context.
       NotImplementedError = Class.new(StandardError)
+
+      NoteableNotFound = Class.new(StandardError)
     end
   end
 end

lib/gitlab/github_import/importer/diff_note_importer.rb

@@ -36,13 +36,6 @@ def execute
           Logger.warn(message: e.message, 'error.class': e.class.name)
 
           import_with_legacy_diff_note
-        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
 
         private

lib/gitlab/github_import/importer/issue_importer.rb

@@ -64,9 +64,6 @@ def create_issue
           issue.validate!
 
           insert_and_return_id(attributes, project.issues)
-        rescue ActiveRecord::InvalidForeignKey
-          # It's possible the project has been deleted since scheduling this
-          # job. In this case we'll just skip creating the issue.
         end
 
         # Stores all issue assignees in the database.

lib/gitlab/github_import/importer/note_importer.rb

@@ -17,7 +17,9 @@ def initialize(note, project, client)
         end
 
         def execute
-          return unless (noteable_id = find_noteable_id)
+          noteable_id = find_noteable_id
+
+          raise Exceptions::NoteableNotFound, 'Error to find noteable_id for note' unless noteable_id
 
           author_id, author_found = user_finder.author_id_for(note)
 
@@ -34,8 +36,7 @@ def execute
             updated_at: note.updated_at
           }
 
-          note = Note.new(attributes.merge(importing: true))
-          note.validate!
+          Note.new(attributes.merge(importing: true)).validate!
 
           # We're using bulk_insert here so we can bypass any callbacks.
           # Running these would result in a lot of unnecessary SQL
@@ -44,9 +45,6 @@ def execute
           # to generate HTML version - you also need to regenerate it in
           # Gitlab::GithubImport::Importer::NoteAttachmentsImporter.
           ApplicationRecord.legacy_bulk_insert(Note.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.
         end
 
         # Returns the ID of the issue or merge request to create the note for.

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

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::GithubImport::Importer::DiffNoteImporter, :aggregate_failures do
+RSpec.describe Gitlab::GithubImport::Importer::DiffNoteImporter, :aggregate_failures, feature_category: :importers do
   let_it_be(:project) { create(:project, :repository) }
   let_it_be(:user) { create(:user) }
 
@@ -80,17 +80,6 @@
       expect(note.author_id).to eq(project.creator_id)
       expect(note.note).to eq("*Created by: #{user.username}*\n\nHello")
     end
-
-    it 'does not import the note when a foreign key error is raised' do
-      stub_user_finder(project.creator_id, false)
-
-      expect(ApplicationRecord)
-        .to receive(:legacy_bulk_insert)
-        .and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key')
-
-      expect { subject.execute }
-        .not_to change(LegacyDiffNote, :count)
-    end
   end
 
   describe '#execute' do

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

@@ -183,21 +183,6 @@
       end
     end
 
-    context 'when the import fails due to a foreign key error' do
-      it 'does not raise any errors' do
-        allow(importer.user_finder)
-          .to receive(:author_id_for)
-          .with(issue)
-          .and_return([user.id, true])
-
-        expect(importer)
-          .to receive(:insert_and_return_id)
-          .and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key')
-
-        expect { importer.create_issue }.not_to raise_error
-      end
-    end
-
     it 'produces a valid Issue' do
       allow(importer.user_finder)
         .to receive(:author_id_for)

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

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::GithubImport::Importer::NoteImporter do
+RSpec.describe Gitlab::GithubImport::Importer::NoteImporter, feature_category: :importers do
   let(:client) { double(:client) }
   let(:project) { create(:project) }
   let(:user) { create(:user) }
@@ -12,13 +12,13 @@
 
   let(:github_note) do
     Gitlab::GithubImport::Representation::Note.new(
+      note_id: 100,
       noteable_id: 1,
       noteable_type: 'Issue',
       author: Gitlab::GithubImport::Representation::User.new(id: 4, login: 'alice'),
       note: note_body,
       created_at: created_at,
-      updated_at: updated_at,
-      github_id: 1
+      updated_at: updated_at
     )
   end
 
@@ -128,34 +128,20 @@
           expect { importer.execute }.to raise_error(ActiveRecord::RecordInvalid)
         end
       end
-    end
-
-    context 'when the noteable does not exist' do
-      it 'does not import the note' do
-        expect(ApplicationRecord).not_to receive(:legacy_bulk_insert)
-
-        importer.execute
-      end
-    end
-
-    context 'when the import fails due to a foreign key error' do
-      it 'does not raise any errors' do
-        issue_row = create(:issue, project: project, iid: 1)
 
+      context 'when noteble_id can not be found' do
+        before do
           allow(importer)
             .to receive(:find_noteable_id)
-          .and_return(issue_row.id)
-
-        allow(importer.user_finder)
-          .to receive(:author_id_for)
-          .with(github_note)
-          .and_return([user.id, true])
-
-        expect(ApplicationRecord)
-          .to receive(:legacy_bulk_insert)
-          .and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key')
+            .and_return(nil)
+        end
 
-        expect { importer.execute }.not_to raise_error
+        it 'raises NoteableNotFound' do
+          expect { importer.execute }.to raise_error(
+            ::Gitlab::GithubImport::Exceptions::NoteableNotFound,
+            'Error to find noteable_id for note'
+          )
+        end
       end
     end
 
@@ -175,13 +161,6 @@
 
       expect(project.notes.take).to be_valid
     end
-
-    # rubocop:disable RSpec/AnyInstanceOf
-    it 'skips markdown field cache callback' do
-      expect_any_instance_of(Note).not_to receive(:refresh_markdown_cache)
-      importer.execute
-    end
-    # rubocop:enable RSpec/AnyInstanceOf
   end
 
   describe '#find_noteable_id' do