diff options
| -rw-r--r-- | lib/gitlab/background_migration/migrate_legacy_uploads.rb | 22 | ||||
| -rw-r--r-- | spec/lib/gitlab/background_migration/migrate_legacy_uploads_spec.rb | 16 |
2 files changed, 32 insertions, 6 deletions
diff --git a/lib/gitlab/background_migration/migrate_legacy_uploads.rb b/lib/gitlab/background_migration/migrate_legacy_uploads.rb index af1ad930aed..e6a68eecf45 100644 --- a/lib/gitlab/background_migration/migrate_legacy_uploads.rb +++ b/lib/gitlab/background_migration/migrate_legacy_uploads.rb @@ -84,9 +84,10 @@ module Gitlab def add_upload_link_to_note_text new_text = "#{note.note} \n #{@uploader.markdown_link}" - note.update!( - note: new_text - ) + # Bypass validations because old data may have invalid + # noteable values. If we fail hard here, we may kill the + # entire background migration, which affects a range of notes. + note.update_attribute(:note, new_text) end def legacy_file_uploader @@ -102,9 +103,10 @@ module Gitlab end def handle_legacy_note_upload - note.note += "\n \n Attachment ##{upload.id} with URL \"#{note.attachment.url}\" failed to migrate \ - for model class #{note.class}. See #{help_doc_link}." - note.save + unless note.note.include?(failed_migration_header) + note.note += failed_migration_text + note.save + end say "MigrateLegacyUploads: LegacyDiffNote ##{note.id} found, can't move the file: #{upload.inspect} for upload ##{upload.id}. See #{help_doc_link}." end @@ -113,6 +115,14 @@ module Gitlab Rails.logger.info(message) end + def failed_migration_text + "\n \n #{failed_migration_header} for model class #{note.class}. See #{help_doc_link}." + end + + def failed_migration_header + "Attachment ##{upload.id} with URL \"#{note.attachment.url}\" failed to migrate" + end + def help_doc_link 'https://docs.gitlab.com/ee/administration/troubleshooting/migrations.html#legacy-upload-migration' end diff --git a/spec/lib/gitlab/background_migration/migrate_legacy_uploads_spec.rb b/spec/lib/gitlab/background_migration/migrate_legacy_uploads_spec.rb index 16b9de6a84e..2aed1b06cb8 100644 --- a/spec/lib/gitlab/background_migration/migrate_legacy_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/migrate_legacy_uploads_spec.rb @@ -193,6 +193,13 @@ describe Gitlab::BackgroundMigration::MigrateLegacyUploads, :migration, schema: described_class.new.perform(start_id, end_id) end + it 'ensures the job is idempotent and does not add duplicate text' do + described_class.new.perform(start_id, end_id) + + expect(note1.reload.note.scan(%r(/uploads/)).count).to eq(1) + expect(legacy_diff_note.reload.note.scan(/failed to migrate/).count).to eq(1) + end + it 'does not remove legacy diff note file' do expect(File.exist?(legacy_upload_diff_note.absolute_path)).to be_truthy end @@ -231,6 +238,15 @@ describe Gitlab::BackgroundMigration::MigrateLegacyUploads, :migration, schema: create_remote_files end + it 'updates notes even if noteable is invalid' do + note1.noteable_id = nil + note1.save(validate: false) + + described_class.new.perform(start_id, end_id) + + expect(note1.reload.note).to include("/uploads/") + end + it_behaves_like 'migrates files correctly' it 'moves legacy uploads to the correct remote location' do |