diff --git a/db/docs/batched_background_migrations/remove_orphaned_vulnerability_notes_batched_migration.yml b/db/docs/batched_background_migrations/remove_orphaned_vulnerability_notes_batched_migration.yml new file mode 100644 index 0000000000000000000000000000000000000000..b2b5c223f909dbf9a6dda6a2a6803acf1085b6bb --- /dev/null +++ b/db/docs/batched_background_migrations/remove_orphaned_vulnerability_notes_batched_migration.yml @@ -0,0 +1,8 @@ +--- +migration_job_name: RemoveOrphanedVulnerabilityNotesBatchedMigration +description: Removes orphaned Notes for which the Vulnerability no longer exists. +feature_category: vulnerability_management +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180624 +milestone: '17.10' +queued_migration_version: 20250304184254 +finalized_by: # version of the migration that finalized this BBM diff --git a/db/post_migrate/20250304184254_queue_remove_orphaned_vulnerability_notes_batched_migration.rb b/db/post_migrate/20250304184254_queue_remove_orphaned_vulnerability_notes_batched_migration.rb new file mode 100644 index 0000000000000000000000000000000000000000..a8e29e1cc53365dc71e7d4ef35eb9b6c17c2db93 --- /dev/null +++ b/db/post_migrate/20250304184254_queue_remove_orphaned_vulnerability_notes_batched_migration.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class QueueRemoveOrphanedVulnerabilityNotesBatchedMigration < Gitlab::Database::Migration[2.2] + milestone '17.10' + + restrict_gitlab_migration gitlab_schema: :gitlab_main + + MIGRATION = "RemoveOrphanedVulnerabilityNotesBatchedMigration" + DELAY_INTERVAL = 2.minutes + BATCH_SIZE = 1000 + SUB_BATCH_SIZE = 100 + + def up + queue_batched_background_migration( + MIGRATION, + :notes, + :id, + job_interval: DELAY_INTERVAL, + batch_size: BATCH_SIZE, + sub_batch_size: SUB_BATCH_SIZE + ) + end + + def down + delete_batched_background_migration(MIGRATION, :notes, :id, []) + end +end diff --git a/db/schema_migrations/20250304184254 b/db/schema_migrations/20250304184254 new file mode 100644 index 0000000000000000000000000000000000000000..86b17c954412aa86febe1b8c956244a6f3a8e4cc --- /dev/null +++ b/db/schema_migrations/20250304184254 @@ -0,0 +1 @@ +0c6eba6946c0434d649293804a2ab50c0f45590ca9d583435aa0aea05a668b9d \ No newline at end of file diff --git a/lib/gitlab/background_migration/remove_orphaned_vulnerability_notes_batched_migration.rb b/lib/gitlab/background_migration/remove_orphaned_vulnerability_notes_batched_migration.rb new file mode 100644 index 0000000000000000000000000000000000000000..5ac51e47132480b1810bf20b5ad9ae94d275fa1a --- /dev/null +++ b/lib/gitlab/background_migration/remove_orphaned_vulnerability_notes_batched_migration.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + class RemoveOrphanedVulnerabilityNotesBatchedMigration < BatchedMigrationJob + operation_name :remove_orphaned_vulnerability_notes + feature_category :vulnerability_management + + def perform + each_sub_batch do |sub_batch| + orphaned_notes = sub_batch + .where(noteable_type: 'Vulnerability') + .where.not(noteable_id: Vulnerability.where(id: sub_batch.pluck(:noteable_id)).pluck(:id)) # rubocop:disable Rails/PluckInWhere -- we want to use pluck here + orphaned_notes.delete_all + end + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/remove_orphaned_vulnerability_notes_batched_migration_spec.rb b/spec/lib/gitlab/background_migration/remove_orphaned_vulnerability_notes_batched_migration_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..db203c7dcab387b1d926d78e916ed2a0ba667168 --- /dev/null +++ b/spec/lib/gitlab/background_migration/remove_orphaned_vulnerability_notes_batched_migration_spec.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BackgroundMigration::RemoveOrphanedVulnerabilityNotesBatchedMigration, feature_category: :vulnerability_management do + let(:migration) do + described_class.new( + batch_table: 'notes', + batch_column: 'id', + sub_batch_size: 100, + pause_ms: 100, + connection: ActiveRecord::Base.connection + ) + end + + let(:user_id) do + table(:users).create!(email: 'author@example.com', notification_email: 'author@example.com', name: 'Author', + username: 'author', projects_limit: 1000).id + end + + let(:organization_id) do + table(:organizations).create!( + name: 'Organization', + path: 'organization').id + end + + let(:namespace_id) do + table(:namespaces).create!( + name: 'Namespace', + path: 'namespace', + organization_id: organization_id).id + end + + let(:project_namespace_id) do + table(:namespaces).create!( + name: 'Project Namespace', + path: 'project-namespace', + organization_id: organization_id).id + end + + let(:project_id) do + table(:projects).create!( + name: 'Project', + path: 'project', + namespace_id: namespace_id, + organization_id: organization_id, + project_namespace_id: project_namespace_id + ).id + end + + let(:note_attributes) do + { + author_id: user_id, + project_id: project_id, + namespace_id: namespace_id, + created_at: Time.current, + updated_at: Time.current + } + end + + before do + allow(Vulnerability).to receive_message_chain(:where, :pluck).and_return([1, 2, 3]) + end + + def create_note(noteable_id:, noteable_type:) + table(:notes).create!( + note: 'Note Content', + noteable_type: noteable_type, + noteable_id: noteable_id, + **note_attributes + ) + end + + describe 'orphaned notes removal' do + # These notes are created before the test runs + let!(:invalid_note_1) { create_note(noteable_id: 4, noteable_type: 'Vulnerability') } + + let!(:invalid_note_2) { create_note(noteable_id: 28, noteable_type: 'Vulnerability') } + + it 'removes orphaned Vulnerability notes' do + expect { migration.perform }.to change { table(:notes).count }.by(-2) + expect(table(:notes).find_by(id: invalid_note_1.id)).to be_blank + expect(table(:notes).find_by(id: invalid_note_2.id)).to be_blank + end + end + + describe 'valid notes retention' do + let!(:valid_note) { create_note(noteable_id: 1, noteable_type: 'Vulnerability') } + + it 'retains valid Vulnerability notes' do + expect { migration.perform }.not_to change { table(:notes).count } + expect(table(:notes).find_by(id: valid_note.id)).to be_present + end + end + + describe 'non-vulnerability notes' do + let!(:issue_note_nil_id) { create_note(noteable_id: nil, noteable_type: 'Issue') } + + let!(:issue_note_with_id) { create_note(noteable_id: 2, noteable_type: 'Issue') } + + it 'does not remove non-Vulnerability notes' do + expect { migration.perform }.not_to change { table(:notes).count } + expect(table(:notes).find_by(id: issue_note_nil_id.id)).to be_present + expect(table(:notes).find_by(id: issue_note_with_id.id)).to be_present + end + end +end diff --git a/spec/migrations/20250304184254_queue_remove_orphaned_vulnerability_notes_batched_migration_spec.rb b/spec/migrations/20250304184254_queue_remove_orphaned_vulnerability_notes_batched_migration_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..607bd3179bdcbd0913e52e3a58ad9cf0f8502089 --- /dev/null +++ b/spec/migrations/20250304184254_queue_remove_orphaned_vulnerability_notes_batched_migration_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe QueueRemoveOrphanedVulnerabilityNotesBatchedMigration, migration: :gitlab_main, feature_category: :vulnerability_management do + let(:migration) { described_class.new } + let(:batched_migration) { described_class::MIGRATION } + + it 'schedules a new batched migration' do + reversible_migration do |migration| + migration.before -> { + expect(batched_migration).not_to have_scheduled_batched_migration + } + + migration.after -> { + expect(batched_migration).to have_scheduled_batched_migration( + gitlab_schema: :gitlab_main, + table_name: :notes, + column_name: :id, + interval: described_class::DELAY_INTERVAL, + batch_size: described_class::BATCH_SIZE, + sub_batch_size: described_class::SUB_BATCH_SIZE + ) + } + end + end + + it 'uses the correct migration class' do + expect(described_class::MIGRATION).to eq('RemoveOrphanedVulnerabilityNotesBatchedMigration') + end + + it 'sets the correct delay interval' do + expect(described_class::DELAY_INTERVAL).to eq(2.minutes) + end + + it 'sets the correct batch size' do + expect(described_class::BATCH_SIZE).to eq(1000) + end + + it 'sets the correct sub-batch size' do + expect(described_class::SUB_BATCH_SIZE).to eq(100) + end +end