Gracefully handle invalid notes in legacy upload migrationsh-fix-legacy-upload-migration

If the database contains notes with invalid noteable values, the
background migration would fail repeatedly, causing duplicate "failed to
migrate" text to appear in LegacyDiffNotes. To avoid this, we now bypass
validations when updating the note text. For LegacyDiffNotes, we also
make the job idempotent so that the duplicate failed migration errors
cannot be added repeatedly.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/62982

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