Force to recreate all diffs on import

If for whatever reason we fail to import MR data,
subsequent run will fail as we try to insert duplicate data.
Instead of trying to recover, lets delete all and retry again.

changelogs/unreleased/recreate-all-diffs-on-import.yml

@@ -0,0 +1,5 @@
+---
+title: Force to recreate all MR diffs on import
+merge_request: 26480
+author:
+type: fixed

lib/gitlab/import/merge_request_helpers.rb

@@ -38,7 +38,6 @@ module Gitlab
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def insert_or_replace_git_data(merge_request, source_branch_sha, target_branch_sha, already_exists = false)
         # These fields are set so we can create the correct merge request
         # diffs.
@@ -47,24 +46,21 @@ module Gitlab
 
         merge_request.keep_around_commit
 
+        # We force to recreate all diffs to replace all existing data
+        # We use `.all` as otherwise `dependent: :nullify` (the default)
+        # takes an effect
+        merge_request.merge_request_diffs.all.delete_all if already_exists
+
         # MR diffs normally use an "after_save" hook to pull data from Git.
         # All of this happens in the transaction started by calling
         # create/save/etc. This in turn can lead to these transactions being
         # held open for much longer than necessary. To work around this we
         # first save the diff, then populate it.
-        diff =
+        diff = merge_request.merge_request_diffs.build
-          if already_exists
-            merge_request.merge_request_diffs.take ||
-              merge_request.merge_request_diffs.build
-          else
-            merge_request.merge_request_diffs.build
-          end
-
         diff.importing = true
         diff.save
         diff.save_git_content
       end
-      # rubocop: enable CodeReuse/ActiveRecord
     end
   end
 end

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

@@ -11,6 +11,7 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
   let(:source_commit) { project.repository.commit('feature') }
   let(:target_commit) { project.repository.commit('master') }
   let(:milestone) { create(:milestone, project: project) }
+  let(:state) { :closed }
 
   let(:pull_request) do
     alice = Gitlab::GithubImport::Representation::User.new(id: 4, login: 'alice')
@@ -26,13 +27,13 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
       source_repository_id: 400,
       target_repository_id: 200,
       source_repository_owner: 'alice',
-      state: :closed,
+      state: state,
       milestone_number: milestone.iid,
       author: alice,
       assignee: alice,
       created_at: created_at,
       updated_at: updated_at,
-      merged_at: merged_at
+      merged_at: state == :closed && merged_at
     )
   end
 
@@ -260,58 +261,63 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
     end
 
     it 'does not create the source branch if merge request is merged' do
-      mr, exists = importer.create_merge_request
+      mr = insert_git_data
-
-      importer.insert_git_data(mr, exists)
 
       expect(project.repository.branch_exists?(mr.source_branch)).to be_falsey
       expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy
     end
 
-    it 'creates the source branch if merge request is open' do
+    context 'when merge request is open' do
-      mr, exists = importer.create_merge_request
+      let(:state) { :opened }
-      mr.state = 'opened'
-      mr.save
 
-      # Ensure the project creator is creating the branches because the
+      it 'creates the source branch' do
-      # merge request author may not have access to push to this
+        # Ensure the project creator is creating the branches because the
-      # repository. The project owner may also be a group.
+        # merge request author may not have access to push to this
-      allow(project.repository).to receive(:add_branch).with(project.creator, anything, anything).and_call_original
+        # repository. The project owner may also be a group.
+        allow(project.repository).to receive(:add_branch).with(project.creator, anything, anything).and_call_original
 
-      importer.insert_git_data(mr, exists)
+        mr = insert_git_data
 
-      expect(project.repository.branch_exists?(mr.source_branch)).to be_truthy
+        expect(project.repository.branch_exists?(mr.source_branch)).to be_truthy
-      expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy
+        expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy
-    end
+      end
 
-    it 'ignores Git errors when creating a branch' do
+      it 'is able to retry on pre-receive errors' do
-      mr, exists = importer.create_merge_request
+        expect(importer).to receive(:insert_or_replace_git_data).twice.and_call_original
-      mr.state = 'opened'
+        expect(project.repository).to receive(:add_branch).and_raise('exception')
-      mr.save
 
-      expect(project.repository).to receive(:add_branch).and_raise(Gitlab::Git::CommandError)
+        expect { insert_git_data }.to raise_error('exception')
-      expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original
 
-      importer.insert_git_data(mr, exists)
+        expect(project.repository).to receive(:add_branch).with(project.creator, anything, anything).and_call_original
 
-      expect(project.repository.branch_exists?(mr.source_branch)).to be_falsey
+        mr = insert_git_data
-      expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy
+
+        expect(project.repository.branch_exists?(mr.source_branch)).to be_truthy
+        expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy
+        expect(mr.merge_request_diffs).to be_one
+      end
+
+      it 'ignores Git command errors when creating a branch' do
+        expect(project.repository).to receive(:add_branch).and_raise(Gitlab::Git::CommandError)
+        expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original
+
+        mr = insert_git_data
+
+        expect(project.repository.branch_exists?(mr.source_branch)).to be_falsey
+        expect(project.repository.branch_exists?(mr.target_branch)).to be_truthy
+      end
     end
 
     it 'creates the merge request diffs' do
-      mr, exists = importer.create_merge_request
+      mr = insert_git_data
-
-      importer.insert_git_data(mr, exists)
 
       expect(mr.merge_request_diffs.exists?).to eq(true)
     end
 
     it 'creates the merge request diff commits' do
-      mr, exists = importer.create_merge_request
+      mr = insert_git_data
 
-      importer.insert_git_data(mr, exists)
+      diff = mr.merge_request_diffs.reload.first
-
-      diff = mr.merge_request_diffs.take
 
       expect(diff.merge_request_diff_commits.exists?).to eq(true)
     end
@@ -327,5 +333,11 @@ describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitlab_redi
         expect(mr.merge_request_diffs.exists?).to eq(true)
       end
     end
+
+    def insert_git_data
+      mr, exists = importer.create_merge_request
+      importer.insert_git_data(mr, exists)
+      mr
+    end
   end
 end