Add NOT NULL constraint to `uuid` column in `security_findings` table

bcdc1fd9 · Michał Zając · Adam Hegyi · eecafe67 · bcdc1fd9 · bcdc1fd9
Commit bcdc1fd9 authored Nov 23, 2021 by Michał Zając Committed by Adam Hegyi Nov 23, 2021
12 changed files
--- a/db/post_migrate/20211110143306_add_not_null_constraint_to_security_findings_uuid.rb
+++ b/db/post_migrate/20211110143306_add_not_null_constraint_to_security_findings_uuid.rb
+# frozen_string_literal: true
+
+class AddNotNullConstraintToSecurityFindingsUuid < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    add_not_null_constraint(
+      :security_findings,
+      :uuid,
+      validate: false
+    )
+  end
+
+  def down
+    remove_not_null_constraint(
+      :security_findings,
+      :uuid
+    )
+  end
+end
--- a/db/post_migrate/20211110151320_add_temporary_index_on_security_findings_uuid.rb
+++ b/db/post_migrate/20211110151320_add_temporary_index_on_security_findings_uuid.rb
+# frozen_string_literal: true
+
+class AddTemporaryIndexOnSecurityFindingsUuid < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  INDEX_NAME = "tmp_index_uuid_is_null"
+
+  def up
+    add_concurrent_index(
+      :security_findings,
+      :id,
+      where: "uuid IS NULL",
+      name: INDEX_NAME
+    )
+  end
+
+  def down
+    remove_concurrent_index_by_name(
+      :security_findings,
+      INDEX_NAME
+    )
+  end
+end
--- a/db/post_migrate/20211110151350_schedule_drop_invalid_security_findings.rb
+++ b/db/post_migrate/20211110151350_schedule_drop_invalid_security_findings.rb
+# frozen_string_literal: true
+
+class ScheduleDropInvalidSecurityFindings < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  MIGRATION = "DropInvalidSecurityFindings"
+  DELAY_INTERVAL = 2.minutes.to_i
+  BATCH_SIZE = 100_000
+  SUB_BATCH_SIZE = 10_000
+
+  def up
+    queue_background_migration_jobs_by_range_at_intervals(
+      define_batchable_model('security_findings').where(uuid: nil),
+      MIGRATION,
+      DELAY_INTERVAL,
+      batch_size: BATCH_SIZE,
+      other_job_arguments: [SUB_BATCH_SIZE],
+      track_jobs: true
+    )
+  end
+
+  def down
+    # no-op
+  end
+end
--- a/db/schema_migrations/20211110143306
+++ b/db/schema_migrations/20211110143306
+7724e5a2c52be99b1b40c449f25abdc23f279f5b0bdaebcfd897c39d295fda41
\ No newline at end of file
--- a/db/schema_migrations/20211110151320
+++ b/db/schema_migrations/20211110151320
+dab6123f19fb44a1566a8de9c760dedec5548dd64e472a180e7748cd7c93eea9
\ No newline at end of file
--- a/db/schema_migrations/20211110151350
+++ b/db/schema_migrations/20211110151350
+f5e69502e582c5f30ba686f8b668d8f0ce5cf8078b0833d2eda67f5ed97ac074
\ No newline at end of file
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -22613,6 +22613,9 @@ ALTER TABLE ONLY chat_teams
 ALTER TABLE vulnerability_scanners
    ADD CONSTRAINT check_37608c9db5 CHECK ((char_length(vendor) <= 255)) NOT VALID;

+ALTER TABLE security_findings
+    ADD CONSTRAINT check_6c2851a8c9 CHECK ((uuid IS NOT NULL)) NOT VALID;
+
 ALTER TABLE sprints
    ADD CONSTRAINT check_ccd8a1eae0 CHECK ((start_date IS NOT NULL)) NOT VALID;

@@ -27722,6 +27725,8 @@ CREATE UNIQUE INDEX tmp_index_on_tmp_project_id_on_namespaces ON namespaces USIN

 CREATE INDEX tmp_index_on_vulnerabilities_non_dismissed ON vulnerabilities USING btree (id) WHERE (state <> 2);

+CREATE INDEX tmp_index_uuid_is_null ON security_findings USING btree (id) WHERE (uuid IS NULL);
+
 CREATE UNIQUE INDEX uniq_pkgs_deb_grp_architectures_on_distribution_id_and_name ON packages_debian_group_architectures USING btree (distribution_id, name);

 CREATE UNIQUE INDEX uniq_pkgs_deb_grp_components_on_distribution_id_and_name ON packages_debian_group_components USING btree (distribution_id, name);
--- a/ee/spec/lib/ee/gitlab/background_migration/populate_uuids_for_security_findings_spec.rb
+++ b/ee/spec/lib/ee/gitlab/background_migration/populate_uuids_for_security_findings_spec.rb
@@ -2,7 +2,7 @@

 require 'spec_helper'

-RSpec.describe ::Gitlab::BackgroundMigration::PopulateUuidsForSecurityFindings do
+RSpec.describe ::Gitlab::BackgroundMigration::PopulateUuidsForSecurityFindings, schema: 20211108211434 do
  let(:users) { table(:users) }
  let(:namespaces) { table(:namespaces) }
  let(:projects) { table(:projects) }

--- a/lib/gitlab/background_migration/drop_invalid_security_findings.rb
+++ b/lib/gitlab/background_migration/drop_invalid_security_findings.rb
+# frozen_string_literal: true
+module Gitlab
+  module BackgroundMigration
+    # Drop rows from security_findings where the uuid is NULL
+    class DropInvalidSecurityFindings
+      # rubocop:disable Style/Documentation
+      class SecurityFinding < ActiveRecord::Base
+        include ::EachBatch
+        self.table_name = 'security_findings'
+        scope :no_uuid, -> { where(uuid: nil) }
+      end
+      # rubocop:enable Style/Documentation
+
+      PAUSE_SECONDS = 0.1
+
+      def perform(start_id, end_id, sub_batch_size)
+        ranged_query = SecurityFinding
+          .where(id: start_id..end_id)
+          .no_uuid
+
+        ranged_query.each_batch(of: sub_batch_size) do |sub_batch|
+          first, last = sub_batch.pluck(Arel.sql('min(id), max(id)')).first
+
+          # The query need to be reconstructed because .each_batch modifies the default scope
+          # See: https://gitlab.com/gitlab-org/gitlab/-/issues/330510
+          SecurityFinding.unscoped
+            .where(id: first..last)
+            .no_uuid
+            .delete_all
+
+          sleep PAUSE_SECONDS
+        end
+
+        mark_job_as_succeeded(start_id, end_id, sub_batch_size)
+      end
+
+      private
+
+      def mark_job_as_succeeded(*arguments)
+        Gitlab::Database::BackgroundMigrationJob.mark_all_as_succeeded(
+          self.class.name.demodulize,
+          arguments
+        )
+      end
+    end
+  end
+end
--- a/spec/lib/gitlab/background_migration/drop_invalid_security_findings_spec.rb
+++ b/spec/lib/gitlab/background_migration/drop_invalid_security_findings_spec.rb
+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::DropInvalidSecurityFindings, schema: 20211108211434 do
+  let(:namespace) { table(:namespaces).create!(name: 'user', path: 'user') }
+  let(:project) { table(:projects).create!(namespace_id: namespace.id) }
+
+  let(:pipelines) { table(:ci_pipelines) }
+  let!(:pipeline) { pipelines.create!(project_id: project.id) }
+
+  let(:ci_builds) { table(:ci_builds) }
+  let!(:ci_build) { ci_builds.create! }
+
+  let(:security_scans) { table(:security_scans) }
+  let!(:security_scan) do
+    security_scans.create!(
+      scan_type: 1,
+      status: 1,
+      build_id: ci_build.id,
+      project_id: project.id,
+      pipeline_id: pipeline.id
+    )
+  end
+
+  let(:vulnerability_scanners) { table(:vulnerability_scanners) }
+  let!(:vulnerability_scanner) { vulnerability_scanners.create!(project_id: project.id, external_id: 'test 1', name: 'test scanner 1') }
+
+  let(:security_findings) { table(:security_findings) }
+  let!(:security_finding_without_uuid) do
+    security_findings.create!(
+      severity: 1,
+      confidence: 1,
+      scan_id: security_scan.id,
+      scanner_id: vulnerability_scanner.id,
+      uuid: nil
+    )
+  end
+
+  let!(:security_finding_with_uuid) do
+    security_findings.create!(
+      severity: 1,
+      confidence: 1,
+      scan_id: security_scan.id,
+      scanner_id: vulnerability_scanner.id,
+      uuid: 'bd95c085-71aa-51d7-9bb6-08ae669c262e'
+    )
+  end
+
+  let(:sub_batch_size) { 10_000 }
+
+  subject { described_class.new.perform(security_finding_without_uuid.id, security_finding_with_uuid.id, sub_batch_size) }
+
+  it 'drops Security::Finding objects with no UUID' do
+    expect { subject }.to change(security_findings, :count).from(2).to(1)
+  end
+end
--- a/spec/migrations/20211110143306_add_not_null_constraint_to_security_findings_uuid_spec.rb
+++ b/spec/migrations/20211110143306_add_not_null_constraint_to_security_findings_uuid_spec.rb
+# frozen_string_literal: true
+require 'spec_helper'
+require_migration!
+
+RSpec.describe AddNotNullConstraintToSecurityFindingsUuid do
+  let_it_be(:security_findings) { table(:security_findings) }
+  let_it_be(:migration) { described_class.new }
+
+  before do
+    allow(migration).to receive(:transaction_open?).and_return(false)
+    allow(migration).to receive(:with_lock_retries).and_yield
+  end
+
+  it 'adds a check constraint' do
+    constraint = security_findings.connection.check_constraints(:security_findings).find { |constraint| constraint.expression == "uuid IS NOT NULL" }
+    expect(constraint).to be_nil
+
+    migration.up
+
+    constraint = security_findings.connection.check_constraints(:security_findings).find { |constraint| constraint.expression == "uuid IS NOT NULL" }
+    expect(constraint).to be_a(ActiveRecord::ConnectionAdapters::CheckConstraintDefinition)
+  end
+end
--- a/spec/migrations/20211110151350_schedule_drop_invalid_security_findings_spec.rb
+++ b/spec/migrations/20211110151350_schedule_drop_invalid_security_findings_spec.rb
+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe ScheduleDropInvalidSecurityFindings, :migration, schema: 20211108211434 do
+  let_it_be(:background_migration_jobs) { table(:background_migration_jobs) }
+
+  let_it_be(:namespace) { table(:namespaces).create!(name: 'user', path: 'user') }
+  let_it_be(:project) { table(:projects).create!(namespace_id: namespace.id) }
+
+  let_it_be(:pipelines) { table(:ci_pipelines) }
+  let_it_be(:pipeline) { pipelines.create!(project_id: project.id) }
+
+  let_it_be(:ci_builds) { table(:ci_builds) }
+  let_it_be(:ci_build) { ci_builds.create! }
+
+  let_it_be(:security_scans) { table(:security_scans) }
+  let_it_be(:security_scan) do
+    security_scans.create!(
+      scan_type: 1,
+      status: 1,
+      build_id: ci_build.id,
+      project_id: project.id,
+      pipeline_id: pipeline.id
+    )
+  end
+
+  let_it_be(:vulnerability_scanners) { table(:vulnerability_scanners) }
+  let_it_be(:vulnerability_scanner) { vulnerability_scanners.create!(project_id: project.id, external_id: 'test 1', name: 'test scanner 1') }
+
+  let_it_be(:security_findings) { table(:security_findings) }
+  let_it_be(:security_finding_without_uuid) do
+    security_findings.create!(
+      severity: 1,
+      confidence: 1,
+      scan_id: security_scan.id,
+      scanner_id: vulnerability_scanner.id,
+      uuid: nil
+    )
+  end
+
+  let_it_be(:security_finding_with_uuid) do
+    security_findings.create!(
+      severity: 1,
+      confidence: 1,
+      scan_id: security_scan.id,
+      scanner_id: vulnerability_scanner.id,
+      uuid: 'bd95c085-71aa-51d7-9bb6-08ae669c262e'
+    )
+  end
+
+  before do
+    stub_const("#{described_class}::BATCH_SIZE", 1)
+    stub_const("#{described_class}::SUB_BATCH_SIZE", 1)
+  end
+
+  around do |example|
+    freeze_time { Sidekiq::Testing.fake! { example.run } }
+  end
+
+  it 'schedules background migrations' do
+    migrate!
+
+    expect(background_migration_jobs.count).to eq(1)
+    expect(background_migration_jobs.first.arguments).to match_array([security_finding_without_uuid.id, security_finding_without_uuid.id, described_class::SUB_BATCH_SIZE])
+
+    expect(BackgroundMigrationWorker.jobs.size).to eq(1)
+    expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, security_finding_without_uuid.id, security_finding_without_uuid.id, described_class::SUB_BATCH_SIZE)
+  end
+end