Merge branch...

Merge branch '477844-resolve-cross-join-issues-in-ee-lib-ee-gitlab-background_migration' into 'master' 

Resolve cross join issues in ee/lib/ee/gitlab/background_migration/migrate_shared_vulnerability_scanners.rb

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167049



Merged-by: Omar Qunsul <oqunsul@gitlab.com>
Approved-by: Pam Artiaga <partiaga@gitlab.com>
Approved-by: Omar Qunsul <oqunsul@gitlab.com>
Co-authored-by: Gregory <11164960-ghavenga@users.noreply.gitlab.com>

.rubocop_todo/layout/empty_line_after_magic_comment.yml

@@ -239,7 +239,6 @@ Layout/EmptyLineAfterMagicComment:
     - 'ee/spec/lib/banzai/filter/issuable_reference_expansion_filter_spec.rb'
     - 'ee/spec/lib/banzai/issuable_extractor_spec.rb'
     - 'ee/spec/lib/ee/api/helpers/members_helpers_spec.rb'
-    - 'ee/spec/lib/ee/gitlab/background_migration/migrate_shared_vulnerability_scanners_spec.rb'
     - 'ee/spec/lib/ee/gitlab/git_access_design_spec.rb'
     - 'ee/spec/lib/ee/gitlab/git_access_snippet_spec.rb'
     - 'ee/spec/lib/ee/gitlab/hook_data/group_member_builder_spec.rb'

.rubocop_todo/lint/redundant_cop_disable_directive.yml

@@ -184,7 +184,6 @@ Lint/RedundantCopDisableDirective:
     - 'ee/lib/ee/gitlab/background_migration/backfill_dismissal_reason_in_vulnerability_reads.rb'
     - 'ee/lib/ee/gitlab/background_migration/backfill_missing_vulnerability_dismissal_details.rb'
     - 'ee/lib/ee/gitlab/background_migration/create_compliance_standards_adherence.rb'
-    - 'ee/lib/ee/gitlab/background_migration/migrate_shared_vulnerability_scanners.rb'
     - 'ee/lib/ee/gitlab/background_migration/populate_denormalized_columns_for_sbom_occurrences.rb'
     - 'ee/lib/ee/gitlab/background_migration/purge_stale_security_scans.rb'
     - 'ee/lib/ee/gitlab/usage_data.rb'

.rubocop_todo/rspec/expect_change.yml

@@ -15,7 +15,6 @@ RSpec/ExpectChange:
     - 'ee/spec/lib/bulk_imports/groups/pipelines/iterations_cadences_pipeline_spec.rb'
     - 'ee/spec/lib/bulk_imports/groups/pipelines/iterations_pipeline_spec.rb'
     - 'ee/spec/lib/ee/feature_spec.rb'
-    - 'ee/spec/lib/ee/gitlab/background_migration/migrate_shared_vulnerability_scanners_spec.rb'
     - 'ee/spec/lib/gitlab/audit/auditor_spec.rb'
     - 'ee/spec/lib/gitlab/auth/ldap/access_spec.rb'
     - 'ee/spec/lib/gitlab/compliance_management/violations/approved_by_committer_spec.rb'

.rubocop_todo/rspec/feature_category.yml

@@ -492,7 +492,6 @@ RSpec/FeatureCategory:
     - 'ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb'
     - 'ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb'
     - 'ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb'
-    - 'ee/spec/lib/ee/gitlab/background_migration/migrate_shared_vulnerability_scanners_spec.rb'
     - 'ee/spec/lib/ee/gitlab/ci/matching/runner_matcher_spec.rb'
     - 'ee/spec/lib/ee/gitlab/ci/pipeline/chain/validate/external_spec.rb'
     - 'ee/spec/lib/ee/gitlab/ci/pipeline/quota/size_spec.rb'

.rubocop_todo/rspec/named_subject.yml

@@ -285,7 +285,6 @@ RSpec/NamedSubject:
     - 'ee/spec/lib/ee/gitlab/background_migration/backfill_iteration_cadence_id_for_boards_spec.rb'
     - 'ee/spec/lib/ee/gitlab/background_migration/delete_invalid_epic_issues_spec.rb'
     - 'ee/spec/lib/ee/gitlab/background_migration/delete_orphaned_transferred_project_approval_rules_spec.rb'
-    - 'ee/spec/lib/ee/gitlab/background_migration/migrate_shared_vulnerability_scanners_spec.rb'
     - 'ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb'
     - 'ee/spec/lib/ee/gitlab/checks/push_rules/branch_check_spec.rb'
     - 'ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb'

.rubocop_todo/style/inline_disable_annotation.yml

@@ -1665,7 +1665,6 @@ Style/InlineDisableAnnotation:
     - 'ee/lib/ee/gitlab/background_migration/create_compliance_standards_adherence.rb'
     - 'ee/lib/ee/gitlab/background_migration/delete_invalid_epic_issues.rb'
     - 'ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb'
-    - 'ee/lib/ee/gitlab/background_migration/migrate_shared_vulnerability_scanners.rb'
     - 'ee/lib/ee/gitlab/background_migration/populate_denormalized_columns_for_sbom_occurrences.rb'
     - 'ee/lib/ee/gitlab/background_migration/populate_latest_pipeline_ids.rb'
     - 'ee/lib/ee/gitlab/background_migration/purge_stale_security_scans.rb'
@@ -1773,7 +1772,6 @@ Style/InlineDisableAnnotation:
     - 'ee/spec/lib/ee/gitlab/background_migration/fix_namespace_ids_of_vulnerability_reads_spec.rb'
     - 'ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb'
     - 'ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb'
-    - 'ee/spec/lib/ee/gitlab/background_migration/migrate_shared_vulnerability_scanners_spec.rb'
     - 'ee/spec/lib/ee/gitlab/background_migration/populate_denormalized_columns_for_sbom_occurrences_spec.rb'
     - 'ee/spec/lib/ee/gitlab/database/docs/docs_spec.rb'
     - 'ee/spec/lib/ee/gitlab/import_export/repo_restorer_spec.rb'

ee/lib/ee/gitlab/background_migration/migrate_shared_vulnerability_scanners.rb

-# frozen_string_literal: true
-
-module EE
-  module Gitlab
-    module BackgroundMigration
-      # Corrects vulnerability findings which are erroneously associated with
-      # vulnerability scanners across across different projects.
-      module MigrateSharedVulnerabilityScanners
-        extend ActiveSupport::Concern
-        extend ::Gitlab::Utils::Override
-
-        class Finding < ::ApplicationRecord # rubocop:disable Style/Documentation
-          include ::EachBatch
-
-          REPORT_TYPES = { cluster_image_scanning: 7, generic: 99 }.freeze
-
-          self.table_name = "vulnerability_occurrences"
-
-          belongs_to :scanner, inverse_of: :findings
-
-          # pipeline fails otherwise
-          validates :details, json_schema: { filename: "vulnerability_finding_details" }, if: false
-
-          def self.to_process
-            where(report_type: REPORT_TYPES.values)
-            .joins(:scanner)
-            .where("vulnerability_occurrences.project_id != vulnerability_scanners.project_id")
-            .allow_cross_joins_across_databases(url: 'https://gitlab.com/groups/gitlab-org/-/epics/14116#identified-cross-joins')
-          end
-        end
-
-        class Scanner < ::ApplicationRecord # rubocop:disable Style/Documentation
-          self.table_name = "vulnerability_scanners"
-
-          has_many :findings, inverse_of: :scanner
-
-          def self.find_or_create_id_for(finding)
-            current_time = Time.zone.now
-            attrs = finding.scanner.attributes.except("id").merge(
-              project_id: finding.project_id,
-              created_at: current_time,
-              updated_at: current_time
-            )
-
-            result = upsert(attrs, unique_by: :index_vulnerability_scanners_on_project_id_and_external_id)
-            result.rows.first.first
-          end
-        end
-
-        class VulnerabilityRead < ::ApplicationRecord # rubocop:disable Style/Documentation
-          self.table_name = "vulnerability_reads"
-        end
-
-        prepended do
-          operation_name :migrate_shared_vulnerability_scanners
-          scope_to ->(relation) { Finding.to_process.merge(relation) }
-        end
-
-        override :perform
-        def perform
-          each_sub_batch do |batch|
-            batch
-              .group_by { |finding| [finding.project_id, finding.scanner.external_id] }.map(&:second)
-              .each do |findings|
-                # all findings within this group should have same `scanner`, so we are taking the first one
-                scanner_id = Scanner.find_or_create_id_for(findings.first)
-
-                Finding.where(id: findings.map(&:id)).update_all(scanner_id: scanner_id)
-
-                VulnerabilityRead.where(uuid: findings.map(&:uuid)).update_all(scanner_id: scanner_id)
-              end
-          end
-        end
-      end
-    end
-  end
-end

ee/spec/lib/ee/gitlab/background_migration/migrate_shared_vulnerability_scanners_spec.rb

-# frozen_string_literal: true
-require 'spec_helper'
-
-RSpec.describe Gitlab::BackgroundMigration::MigrateSharedVulnerabilityScanners, :migration do
-  # `described_class` resolves to different values depending on scope
-  # rubocop: disable RSpec/DescribedClass
-  let(:migration) { Gitlab::BackgroundMigration::MigrateSharedVulnerabilityScanners }
-  # rubocop: enable RSpec/DescribedClass
-
-  let(:namespaces) { table(:namespaces) }
-  let(:projects) { table(:projects) }
-  let(:identifiers) { table(:vulnerability_identifiers) }
-  let(:vulnerabilities) { table(:vulnerabilities) }
-  let(:vulnerability_reads) { table(:vulnerability_reads) }
-  let(:users) { table(:users) }
-  let(:findings) { migration::Finding }
-  let(:scanners) { migration::Scanner }
-
-  let(:migration_attrs) do
-    {
-      start_id: findings.minimum(:id),
-      end_id: findings.maximum(:id),
-      batch_table: :vulnerability_occurrences,
-      batch_column: :id,
-      sub_batch_size: 100,
-      pause_ms: 0,
-      connection: ApplicationRecord.connection
-    }
-  end
-
-  let(:namespace_a) { namespaces.create!(name: "test-1", path: "test-1") }
-  let(:namespace_b) { namespaces.create!(name: "test-2", path: "test-2") }
-  let(:project) { projects.create!(namespace_id: namespace_a.id, project_namespace_id: namespace_a.id) }
-  let(:other_project) { projects.create!(namespace_id: namespace_b.id, project_namespace_id: namespace_b.id) }
-
-  def scanner(project, overrides = {})
-    attrs = {
-      project_id: project.id,
-      external_id: "starboard_trivy",
-      name: "Trivy (via Starboard Operator)"
-    }.merge(overrides)
-
-    scanners.create!(attrs)
-  end
-
-  def finding(project, scanner, overrides = {})
-    attrs = {
-      project_id: project.id,
-      scanner_id: scanner.id,
-      severity: "medium",
-      confidence: "unknown",
-      report_type: 99,
-      primary_identifier_id: identifier(project).id,
-      project_fingerprint: SecureRandom.hex(20),
-      location_fingerprint: SecureRandom.hex(20),
-      uuid: SecureRandom.uuid,
-      name: "CVE-2018-1234",
-      raw_metadata: "{}",
-      metadata_version: "cluster_image_scanning:1.0"
-    }.merge(overrides)
-
-    findings.create!(attrs)
-  end
-
-  def vulnerability(project, overrides = {})
-    attrs = {
-      title: "test",
-      severity: 6, # high
-      confidence: 6, # high
-      report_type: 0, # sast
-      description: "test",
-      project_id: project.id,
-      author_id: overrides.fetch(:author_id) { user.id },
-      finding_id: overrides.fetch(:finding_id)
-    }
-
-    vulnerabilities.create!(attrs)
-  end
-
-  def user(overrides = {})
-    attrs = {
-      email: "test@example.com",
-      notification_email: "test@example.com",
-      name: "test",
-      username: "test",
-      state: "active",
-      projects_limit: 10
-    }.merge(overrides)
-
-    users.create!(attrs)
-  end
-
-  def identifier(project, overrides = {})
-    attrs = {
-      project_id: project.id,
-      external_id: "CVE-2018-1234",
-      external_type: "CVE",
-      name: "CVE-2018-1234",
-      fingerprint: SecureRandom.hex(20)
-    }.merge(overrides)
-
-    identifiers.create!(attrs)
-  end
-
-  describe described_class::Finding do
-    describe ".to_process" do
-      let(:project_scanner) { scanner(project) }
-      let!(:cluster_image_scanning_finding) { finding(project, scanner(other_project), report_type: 7) }
-      let!(:generic_finding) { finding(project, project_scanner, report_type: 99) }
-      let!(:secret_detection_finding) { finding(project, project_scanner, report_type: 4) }
-
-      subject { described_class.to_process.pluck(:id) }
-
-      it "returns findings with report type cluster image scanning or generic" do
-        expect(subject).to match_array([cluster_image_scanning_finding].map(&:id))
-      end
-    end
-  end
-
-  describe described_class::Scanner do
-    describe "::find_or_create_id_for" do
-      let!(:existing_finding) { finding(project, existing_scanner) }
-
-      subject { described_class.find_or_create_id_for(existing_finding) }
-
-      context "when finding has matching scanner" do
-        let(:existing_scanner) { scanner(project) }
-
-        it "does not create a new scanner" do
-          expect { subject }.not_to change(scanners, :count)
-        end
-
-        it "returns the scanner ID" do
-          expect(subject).to be(existing_scanner.id)
-        end
-      end
-
-      context "when finding has mismatching scanner" do
-        let(:existing_scanner) { scanner(other_project) }
-
-        it "creates a new scanner" do
-          expect { subject }.to change(scanners, :count).by(1)
-        end
-
-        it "sets attributes" do
-          scanner = scanners.find(subject)
-
-          expect(scanner).to have_attributes(
-            project_id: project.id,
-            external_id: existing_scanner.external_id,
-            name: existing_scanner.name,
-            vendor: existing_scanner.vendor
-          )
-        end
-      end
-    end
-  end
-
-  describe "#perform" do
-    let(:shared_scanner) { scanner(project) }
-    let!(:correct_finding) { finding(project, shared_scanner) }
-    let!(:incorrect_finding) { finding(other_project, shared_scanner) }
-
-    subject { described_class.new(**migration_attrs).perform }
-
-    it "creates new scanners for incorrect findings" do
-      expect { subject }.to change(scanners, :count).by(1)
-    end
-
-    it "creates scanners with correct attributes" do
-      subject
-
-      scanner = scanners.find(incorrect_finding.reload.scanner_id)
-      expect(scanner).to have_attributes(
-        project_id: other_project.id,
-        external_id: shared_scanner.external_id,
-        name: shared_scanner.name,
-        vendor: shared_scanner.vendor
-      )
-    end
-
-    it "updates erroneous associations" do
-      subject
-
-      expect(incorrect_finding.reload.scanner_id).to be(scanners.last.id)
-    end
-
-    it "does not alter correct findings" do
-      expect { subject }.not_to change { correct_finding.reload.attributes }
-    end
-
-    context "with existing scanner" do
-      context "with matching external ID" do
-        let!(:existing_scanner) { scanner(other_project) }
-
-        it "does not create a new scanner" do
-          expect { subject }.not_to change(scanners, :count)
-        end
-
-        it "reuses the scanner" do
-          expect { subject }.to change { incorrect_finding.reload.scanner_id }.to(existing_scanner.id)
-        end
-      end
-
-      context "with mismatching external ID" do
-        let!(:existing_scanner) { scanner(other_project, external_id: "foobar") }
-
-        it "creates a new scanner" do
-          expect { subject }.to change(scanners, :count).by(1)
-        end
-
-        it "does not reuse the scanner" do
-          subject
-
-          expect(incorrect_finding.reload.scanner_id).to be(scanners.last.id)
-        end
-      end
-    end
-
-    context "with associated vulnerability" do
-      let!(:user_a) { user(email: "test1@example.com", username: "test1") }
-      let!(:user_b) { user(email: "test2@example.com", username: "test2") }
-
-      let!(:correct_finding) { finding(project, shared_scanner) }
-      let!(:incorrect_finding) { finding(other_project, shared_scanner) }
-
-      let(:vulnerability_a) { vulnerability(project, author_id: user_a.id, finding_id: correct_finding.id) }
-      let(:vulnerability_b) { vulnerability(other_project, author_id: user_b.id, finding_id: incorrect_finding.id) }
-
-      before do
-        correct_finding.update!(vulnerability_id: vulnerability_a.id)
-        incorrect_finding.update!(vulnerability_id: vulnerability_b.id)
-      end
-
-      it "updates vulnerability reads" do
-        subject
-
-        scanner_a = scanners.find_by!(project_id: project.id)
-        scanner_b = scanners.find_by!(project_id: other_project.id)
-
-        vulnerability_reads.find_by!(
-          project_id: project.id, vulnerability_id: vulnerability_a.id, scanner_id: scanner_a.id
-        )
-        vulnerability_reads.find_by!(
-          project_id: other_project.id, vulnerability_id: vulnerability_b.id, scanner_id: scanner_b.id
-        )
-      end
-    end
-  end
-end

spec/support/rspec_order_todo.yml

@@ -856,7 +856,6 @@
 - './ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_check_progress_spec.rb'
 - './ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_in_batch_spec.rb'
 - './ee/spec/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules_spec.rb'
-- './ee/spec/lib/ee/gitlab/background_migration/migrate_shared_vulnerability_scanners_spec.rb'
 - './ee/spec/lib/ee/gitlab/checks/push_rule_check_spec.rb'
 - './ee/spec/lib/ee/gitlab/checks/push_rules/branch_check_spec.rb'
 - './ee/spec/lib/ee/gitlab/checks/push_rules/commit_check_spec.rb'