Merge branch 'sh-validate-path-project-import' into 'master'

Avoid leaving a push event empty if payload cannot be created

See merge request gitlab-org/gitlab-ce!16214

changelogs/unreleased/sh-validate-path-project-import.yml

+---
+title: Avoid leaving a push event empty if payload cannot be created
+merge_request:
+author:
+type: fixed

lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb

@@ -128,18 +128,21 @@ module Gitlab
       end
 
       def process_event(event)
+        ActiveRecord::Base.transaction do
           replicate_event(event)
           create_push_event_payload(event) if event.push_event?
         end
+      rescue ActiveRecord::InvalidForeignKey => e
+        # A foreign key error means the associated event was removed. In this
+        # case we'll just skip migrating the event.
+        Rails.logger.error("Unable to migrate event #{event.id}: #{e}")
+      end
 
       def replicate_event(event)
         new_attributes = event.attributes
           .with_indifferent_access.except(:title, :data)
 
         EventForMigration.create!(new_attributes)
-      rescue ActiveRecord::InvalidForeignKey
-        # A foreign key error means the associated event was removed. In this
-        # case we'll just skip migrating the event.
       end
 
       def create_push_event_payload(event)
@@ -156,9 +159,6 @@ module Gitlab
           ref: event.trimmed_ref_name,
           commit_title: event.commit_title
         )
-      rescue ActiveRecord::InvalidForeignKey
-        # A foreign key error means the associated event was removed. In this
-        # case we'll just skip migrating the event.
       end
 
       def find_events(start_id, end_id)

spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb

@@ -281,6 +281,17 @@ describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migrati
 
       migration.process_event(event)
     end
+
+    it 'handles an error gracefully' do
+      event1 = create_push_event(project, author, { commits: [] })
+
+      expect(migration).to receive(:replicate_event).and_call_original
+      expect(migration).to receive(:create_push_event_payload).and_raise(ActiveRecord::InvalidForeignKey, 'invalid foreign key')
+
+      migration.process_event(event1)
+
+      expect(described_class::EventForMigration.all.count).to eq(0)
+    end
   end
 
   describe '#replicate_event' do
@@ -335,9 +346,8 @@ describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migrati
     it 'does not create push event payloads for removed events' do
       allow(event).to receive(:id).and_return(-1)
 
-      payload = migration.create_push_event_payload(event)
+      expect { migration.create_push_event_payload(event) }.to raise_error(ActiveRecord::InvalidForeignKey)
 
-      expect(payload).to be_nil
       expect(PushEventPayload.count).to eq(0)
     end