diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2018-01-04 15:17:20 +0300 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2018-01-04 15:17:20 +0300 |
commit | 51562aafc782f7eb65152f0551d9cb41268aeae2 (patch) | |
tree | d8adb557377df2d51f6936b55b9d81fbd1c4da77 | |
parent | a0ae88613598a2b0fddad4e7a6f779827257f103 (diff) | |
parent | 57dc5a521bbdd94cc2eab34f22e2b6fac9e8bd55 (diff) |
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
3 files changed, 25 insertions, 10 deletions
diff --git a/changelogs/unreleased/sh-validate-path-project-import.yml b/changelogs/unreleased/sh-validate-path-project-import.yml new file mode 100644 index 00000000000..acad66c0ab2 --- /dev/null +++ b/changelogs/unreleased/sh-validate-path-project-import.yml @@ -0,0 +1,5 @@ +--- +title: Avoid leaving a push event empty if payload cannot be created +merge_request: +author: +type: fixed diff --git a/lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb b/lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb index 84ac00f1a5c..7088aa0860a 100644 --- a/lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb +++ b/lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb @@ -128,8 +128,14 @@ module Gitlab end def process_event(event) - replicate_event(event) - create_push_event_payload(event) if event.push_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) @@ -137,9 +143,6 @@ module Gitlab .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) diff --git a/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb b/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb index 7351d45336a..5432d270555 100644 --- a/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb +++ b/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 |