Commit 57dc5a52 authored by Stan Hu's avatar Stan Hu

Avoid leaving a push event empty if payload cannot be created

If the payload cannot be created for some reason, we could be left with a nil
push event payload, which causes Error 500s when viewing the dashboard. Guard
against this error and log when it happens.

Avoids problems seen in #38823
parent 54bc270f
---
title: Avoid leaving a push event empty if payload cannot be created
merge_request:
author:
type: fixed
...@@ -128,18 +128,21 @@ module Gitlab ...@@ -128,18 +128,21 @@ module Gitlab
end end
def process_event(event) def process_event(event)
ActiveRecord::Base.transaction do
replicate_event(event) replicate_event(event)
create_push_event_payload(event) if event.push_event? create_push_event_payload(event) if event.push_event?
end 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) def replicate_event(event)
new_attributes = event.attributes new_attributes = event.attributes
.with_indifferent_access.except(:title, :data) .with_indifferent_access.except(:title, :data)
EventForMigration.create!(new_attributes) 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 end
def create_push_event_payload(event) def create_push_event_payload(event)
...@@ -156,9 +159,6 @@ module Gitlab ...@@ -156,9 +159,6 @@ module Gitlab
ref: event.trimmed_ref_name, ref: event.trimmed_ref_name,
commit_title: event.commit_title 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 end
def find_events(start_id, end_id) def find_events(start_id, end_id)
......
...@@ -281,6 +281,17 @@ describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migrati ...@@ -281,6 +281,17 @@ describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migrati
migration.process_event(event) migration.process_event(event)
end 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 end
describe '#replicate_event' do describe '#replicate_event' do
...@@ -335,9 +346,8 @@ describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migrati ...@@ -335,9 +346,8 @@ describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads, :migrati
it 'does not create push event payloads for removed events' do it 'does not create push event payloads for removed events' do
allow(event).to receive(:id).and_return(-1) 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) expect(PushEventPayload.count).to eq(0)
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment