Merge branch '6717_update_vulnerabilities_db_model' into 'master'

Update DB model for security reports

See merge request gitlab-org/gitlab-ee!7779

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180926140319) do
+ActiveRecord::Schema.define(version: 20180930171532) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -2997,7 +2997,6 @@ ActiveRecord::Schema.define(version: 20180926140319) do
     t.datetime_with_timezone "updated_at", null: false
     t.integer "occurrence_id", limit: 8, null: false
     t.integer "identifier_id", limit: 8, null: false
-    t.boolean "primary", default: false, null: false
   end
 
   add_index "vulnerability_occurrence_identifiers", ["identifier_id"], name: "index_vulnerability_occurrence_identifiers_on_identifier_id", using: :btree
@@ -3012,10 +3011,10 @@ ActiveRecord::Schema.define(version: 20180926140319) do
     t.integer "pipeline_id", null: false
     t.integer "project_id", null: false
     t.integer "scanner_id", limit: 8, null: false
-    t.binary "first_seen_in_commit_sha", null: false
     t.binary "project_fingerprint", null: false
     t.binary "location_fingerprint", null: false
     t.binary "primary_identifier_fingerprint", null: false
+    t.string "uuid", limit: 36, null: false
     t.string "ref", null: false
     t.string "name", null: false
     t.string "metadata_version", null: false
@@ -3023,8 +3022,9 @@ ActiveRecord::Schema.define(version: 20180926140319) do
   end
 
   add_index "vulnerability_occurrences", ["pipeline_id"], name: "index_vulnerability_occurrences_on_pipeline_id", using: :btree
-  add_index "vulnerability_occurrences", ["project_id", "ref", "scanner_id", "primary_identifier_fingerprint", "location_fingerprint"], name: "index_vulnerability_occurrences_on_unique_keys", unique: true, using: :btree
+  add_index "vulnerability_occurrences", ["project_id", "ref", "primary_identifier_fingerprint", "location_fingerprint", "pipeline_id", "scanner_id"], name: "index_vulnerability_occurrences_on_unique_keys", unique: true, using: :btree
   add_index "vulnerability_occurrences", ["scanner_id"], name: "index_vulnerability_occurrences_on_scanner_id", using: :btree
+  add_index "vulnerability_occurrences", ["uuid"], name: "index_vulnerability_occurrences_on_uuid", unique: true, using: :btree
 
   create_table "vulnerability_scanners", id: :bigserial, force: :cascade do |t|
     t.datetime_with_timezone "created_at", null: false

ee/app/models/vulnerabilities/occurrence.rb

@@ -18,7 +18,6 @@ module Vulnerabilities
       critical: 7
     }.with_indifferent_access.freeze
 
-    sha_attribute :first_seen_in_commit_sha
     sha_attribute :project_fingerprint
     sha_attribute :primary_identifier_fingerprint
     sha_attribute :location_fingerprint
@@ -40,15 +39,15 @@ module Vulnerabilities
     validates :scanner, presence: true
     validates :project, presence: true
     validates :pipeline, presence: true
+    validates :uuid, presence: true
     validates :ref, presence: true
 
-    validates :first_seen_in_commit_sha, presence: true
     validates :project_fingerprint, presence: true
     validates :primary_identifier_fingerprint, presence: true
     validates :location_fingerprint, presence: true
     # Uniqueness validation doesn't work with binary columns, so save this useless query. It is enforce by DB constraint anyway.
     # TODO: find out why it fails
-    # validates :location_fingerprint, presence: true, uniqueness: { scope: [:primary_identifier_fingerprint, :scanner_id, :ref, :project_id] }
+    # validates :location_fingerprint, presence: true, uniqueness: { scope: [:primary_identifier_fingerprint, :scanner_id, :ref, :pipeline_id, :project_id] }
     validates :name, presence: true
     validates :report_type, presence: true
     validates :severity, presence: true, inclusion: { in: LEVELS.keys }

ee/app/models/vulnerabilities/occurrence_identifier.rb

@@ -10,6 +10,5 @@ module Vulnerabilities
     validates :occurrence, presence: true
     validates :identifier, presence: true
     validates :identifier_id, uniqueness: { scope: [:occurrence_id] }
-    validates :occurrence_id, uniqueness: true, if: :primary
   end
 end

ee/changelogs/unreleased/6717_update_vulnerabilities_db_model.yml

+---
+title: Update DB model for security reports
+merge_request:
+author:
+type: performance

ee/db/migrate/20180930171532_recreate_vulnerability_occurrences_and_vulnerability_occurrence_identifiers.rb

+# frozen_string_literal: true
+
+class RecreateVulnerabilityOccurrencesAndVulnerabilityOccurrenceIdentifiers < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    drop_table :vulnerability_occurrence_identifiers
+    drop_table :vulnerability_occurrences
+
+    create_table :vulnerability_occurrences, id: :bigserial  do |t|
+      t.timestamps_with_timezone null: false
+
+      t.integer :severity, null: false, limit: 2
+      t.integer :confidence, null: false, limit: 2
+      t.integer :report_type, null: false, limit: 2
+
+      t.integer :pipeline_id, null: false
+      t.foreign_key :ci_pipelines, column: :pipeline_id, on_delete: :cascade
+      t.references :project, null: false, foreign_key: { on_delete: :cascade }
+
+      t.bigint :scanner_id, null: false
+      t.foreign_key :vulnerability_scanners, column: :scanner_id, on_delete: :cascade
+
+      t.binary :project_fingerprint, null: false, limit: 20
+      t.binary :location_fingerprint, null: false, limit: 20
+      t.binary :primary_identifier_fingerprint, null: false, limit: 20
+
+      t.string :uuid, null: false, limit: 36
+      t.string :ref, null: false
+      t.string :name, null: false
+      t.string :metadata_version, null: false
+      t.text :raw_metadata, null: false
+
+      t.index :pipeline_id
+      t.index :scanner_id
+      t.index :uuid, unique: true
+
+      t.index [:project_id, :ref, :primary_identifier_fingerprint, :location_fingerprint, :pipeline_id, :scanner_id],
+        unique: true,
+        name: 'index_vulnerability_occurrences_on_unique_keys',
+        length: { location_fingerprint: 20, primary_identifier_fingerprint: 20 }
+    end
+
+    create_table :vulnerability_occurrence_identifiers, id: :bigserial do |t|
+      t.timestamps_with_timezone null: false
+
+      t.bigint :occurrence_id, null: false
+      t.foreign_key :vulnerability_occurrences, column: :occurrence_id, on_delete: :cascade
+      t.bigint :identifier_id, null: false
+      t.foreign_key :vulnerability_identifiers, column: :identifier_id, on_delete: :cascade
+
+      t.index :identifier_id
+      t.index [:occurrence_id, :identifier_id],
+        unique: true,
+        name: 'index_vulnerability_occurrence_identifiers_on_unique_keys'
+    end
+  end
+
+  def down
+    drop_table :vulnerability_occurrence_identifiers
+    drop_table :vulnerability_occurrences
+
+    create_table :vulnerability_occurrences, id: :bigserial  do |t|
+      t.timestamps_with_timezone null: false
+
+      t.integer :severity, null: false, limit: 2
+      t.integer :confidence, null: false, limit: 2
+      t.integer :report_type, null: false, limit: 2
+
+      t.integer :pipeline_id, null: false
+      t.foreign_key :ci_pipelines, column: :pipeline_id, on_delete: :cascade
+      t.references :project, null: false, foreign_key: { on_delete: :cascade }
+
+      t.bigint :scanner_id, null: false
+      t.foreign_key :vulnerability_scanners, column: :scanner_id, on_delete: :cascade
+
+      t.binary :first_seen_in_commit_sha, null: false, limit: 20
+      t.binary :project_fingerprint, null: false, limit: 20
+      t.binary :location_fingerprint, null: false, limit: 20
+      t.binary :primary_identifier_fingerprint, null: false, limit: 20
+
+      t.string :ref, null: false
+      t.string :name, null: false
+      t.string :metadata_version, null: false
+      t.text :raw_metadata, null: false
+
+      t.index :pipeline_id
+      t.index :scanner_id
+      t.index [:project_id, :ref, :scanner_id, :primary_identifier_fingerprint, :location_fingerprint],
+        unique: true,
+        name: 'index_vulnerability_occurrences_on_unique_keys',
+        length: { location_fingerprint: 20, primary_identifier_fingerprint: 20 }
+    end
+
+    create_table :vulnerability_occurrence_identifiers, id: :bigserial do |t|
+      t.timestamps_with_timezone null: false
+
+      t.bigint :occurrence_id, null: false
+      t.foreign_key :vulnerability_occurrences, column: :occurrence_id, on_delete: :cascade
+      t.bigint :identifier_id, null: false
+      t.foreign_key :vulnerability_identifiers, column: :identifier_id, on_delete: :cascade
+
+      t.boolean :primary, null: false, default: false
+
+      t.index :identifier_id
+      t.index [:occurrence_id, :identifier_id],
+        unique: true,
+        name: 'index_vulnerability_occurrence_identifiers_on_unique_keys'
+    end
+  end
+end

ee/spec/factories/vulnerabilities/occurrences.rb

@@ -6,7 +6,7 @@ FactoryBot.define do
     project
     pipeline factory: :ci_pipeline
     ref 'master'
-    first_seen_in_commit_sha '52d084cede3db8fafcd6b8ae382ddf1970da3b7f'
+    uuid 'a7342ca9-494e-457f-88e7-e65e145cc392'
     project_fingerprint '4e5b6966dd100170b4b1ad599c7058cce91b57b4'
     primary_identifier_fingerprint '4e5b6966dd100170b4b1ad599c7058cce91b57b4'
     location_fingerprint '4e5b6966dd100170b4b1ad599c7058cce91b57b4'

ee/spec/models/vulnerabilities/occurrence_identifier_spec.rb

@@ -14,21 +14,5 @@ describe Vulnerabilities::OccurrenceIdentifier do
     it { is_expected.to validate_presence_of(:occurrence) }
     it { is_expected.to validate_presence_of(:identifier) }
     it { is_expected.to validate_uniqueness_of(:identifier_id).scoped_to(:occurrence_id) }
-
-    context 'when primary' do
-      before do
-        allow_any_instance_of(described_class).to receive(:primary).and_return(true)
-      end
-
-      it { is_expected.to validate_uniqueness_of(:occurrence_id) }
-    end
-
-    context 'when not primary' do
-      before do
-        allow_any_instance_of(described_class).to receive(:primary).and_return(false)
-      end
-
-      it { is_expected.not_to validate_uniqueness_of(:occurrence_id) }
-    end
   end
 end

ee/spec/models/vulnerabilities/occurrence_spec.rb

@@ -20,7 +20,7 @@ describe Vulnerabilities::Occurrence do
     it { is_expected.to validate_presence_of(:project) }
     it { is_expected.to validate_presence_of(:pipeline) }
     it { is_expected.to validate_presence_of(:ref) }
-    it { is_expected.to validate_presence_of(:first_seen_in_commit_sha) }
+    it { is_expected.to validate_presence_of(:uuid) }
     it { is_expected.to validate_presence_of(:project_fingerprint) }
     it { is_expected.to validate_presence_of(:primary_identifier_fingerprint) }
     it { is_expected.to validate_presence_of(:location_fingerprint) }
@@ -32,13 +32,11 @@ describe Vulnerabilities::Occurrence do
     it { is_expected.to validate_inclusion_of(:severity).in_array(described_class::LEVELS.keys) }
     it { is_expected.to validate_presence_of(:confidence) }
     it { is_expected.to validate_inclusion_of(:confidence).in_array(described_class::LEVELS.keys) }
-    # Uniqueness validation doesn't work with binary columns. See TODO in class file
-    # it { is_expected.to validate_uniqueness_of(:ref).scoped_to(:primary_identifier_fingerprint, :location_fingerprint, :scanner_id, :ref, :project_id) }
   end
 
   context 'database uniqueness' do
     let(:occurrence) { create(:vulnerabilities_occurrence) }
-    let(:new_occurrence) { occurrence.dup }
+    let(:new_occurrence) { occurrence.dup.tap { |o| o.uuid = SecureRandom.uuid } }
 
     it "when all index attributes are identical" do
       expect { new_occurrence.save! }.to raise_error(ActiveRecord::RecordNotUnique)
@@ -52,6 +50,7 @@ describe Vulnerabilities::Occurrence do
         :primary_identifier_fingerprint | -> { '005b6966dd100170b4b1ad599c7058cce91b57b4' }
         :ref | -> { 'another_ref' }
         :scanner | -> { create(:vulnerabilities_scanner) }
+        :pipeline | -> { create(:ci_pipeline) }
         :project | -> { create(:project) }
       end