Merge branch '271408_introduce_latest_pipeline_id_for_vulnerability_statistics_table' into 'master'

Add `latest_pipeline_id` column to `vulnerability_statistics` table

See merge request gitlab-org/gitlab!62610

db/migrate/20210525184900_add_latest_pipeline_id_into_vulnerability_statistics_table.rb

+# frozen_string_literal: true
+
+class AddLatestPipelineIdIntoVulnerabilityStatisticsTable < ActiveRecord::Migration[6.0]
+  def change
+    add_column :vulnerability_statistics, :latest_pipeline_id, :bigint
+  end
+end

db/migrate/20210526181820_add_index_to_vulnerability_statistics_on_latest_pipeline_id.rb

+# frozen_string_literal: true
+
+class AddIndexToVulnerabilityStatisticsOnLatestPipelineId < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  INDEX_NAME = 'index_vulnerability_statistics_on_latest_pipeline_id'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :vulnerability_statistics, :latest_pipeline_id, name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :vulnerability_statistics, INDEX_NAME
+  end
+end

db/migrate/20210526181821_add_foreign_key_for_latest_pipeline_id_to_ci_pipelines.rb

+# frozen_string_literal: true
+
+class AddForeignKeyForLatestPipelineIdToCiPipelines < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :vulnerability_statistics, :ci_pipelines, column: :latest_pipeline_id, on_delete: :nullify
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key_if_exists :vulnerability_statistics, :ci_pipelines
+    end
+  end
+end

db/schema_migrations/20210525184900

+ae91ea7481ea21ce29b4c0697f77fd83017c36d913739ed67e5c907a48c56f69
\ No newline at end of file

db/schema_migrations/20210526181820

+e72471e63dc108939473232437eda4c718382630c1173ae20023002d382e5ffa
\ No newline at end of file

db/schema_migrations/20210526181821

+3c53d85bec154ec68a23841d37317d10fa6c7c846bc5f54f5b7876081105ac7b
\ No newline at end of file

db/structure.sql

@@ -19249,7 +19249,8 @@ CREATE TABLE vulnerability_statistics (
     low integer DEFAULT 0 NOT NULL,
     unknown integer DEFAULT 0 NOT NULL,
     info integer DEFAULT 0 NOT NULL,
-    letter_grade smallint NOT NULL
+    letter_grade smallint NOT NULL,
+    latest_pipeline_id bigint
 );
 
 CREATE SEQUENCE vulnerability_statistics_id_seq
@@ -24869,6 +24870,8 @@ CREATE UNIQUE INDEX index_vulnerability_remediations_on_project_id_and_checksum
 
 CREATE UNIQUE INDEX index_vulnerability_scanners_on_project_id_and_external_id ON vulnerability_scanners USING btree (project_id, external_id);
 
+CREATE INDEX index_vulnerability_statistics_on_latest_pipeline_id ON vulnerability_statistics USING btree (latest_pipeline_id);
+
 CREATE INDEX index_vulnerability_statistics_on_letter_grade ON vulnerability_statistics USING btree (letter_grade);
 
 CREATE UNIQUE INDEX index_vulnerability_statistics_on_unique_project_id ON vulnerability_statistics USING btree (project_id);
@@ -25973,6 +25976,9 @@ ALTER TABLE ONLY sprints
 ALTER TABLE ONLY application_settings
     ADD CONSTRAINT fk_e8a145f3a7 FOREIGN KEY (instance_administrators_group_id) REFERENCES namespaces(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY vulnerability_statistics
+    ADD CONSTRAINT fk_e8b13c928f FOREIGN KEY (latest_pipeline_id) REFERENCES ci_pipelines(id) ON DELETE SET NULL;
+
 ALTER TABLE ONLY ci_triggers
     ADD CONSTRAINT fk_e8e10d1964 FOREIGN KEY (owner_id) REFERENCES users(id) ON DELETE CASCADE;
 

ee/app/models/ee/project.rb

@@ -305,12 +305,11 @@ module EE
       namespace.store_security_reports_available? || public?
     end
 
+    # The `only_successful` flag is wrong here and will be addressed by
+    # https://gitlab.com/gitlab-org/gitlab/-/issues/331950
+    # We will also remove the fallback to `latest_not_ingested_security_pipeline` method with that issue.
     def latest_pipeline_with_security_reports(only_successful: false)
-      pipeline_scope = all_pipelines.newest_first(ref: default_branch)
-      pipeline_scope = pipeline_scope.success if only_successful
-
-      pipeline_scope.with_reports(::Ci::JobArtifact.security_reports).first ||
-        pipeline_scope.with_legacy_security_reports.first
+      (!only_successful && latest_ingested_security_pipeline) || latest_not_ingested_security_pipeline(only_successful)
     end
 
     def latest_pipeline_with_reports(reports)
@@ -850,5 +849,17 @@ module EE
     def group_deletion_mode_configured?
       group && group.namespace_settings.delayed_project_removal?
     end
+
+    def latest_ingested_security_pipeline
+      vulnerability_statistic&.pipeline
+    end
+
+    def latest_not_ingested_security_pipeline(only_successful)
+      pipeline_scope = all_pipelines.newest_first(ref: default_branch)
+      pipeline_scope = pipeline_scope.success if only_successful
+
+      pipeline_scope.with_reports(::Ci::JobArtifact.security_reports).first ||
+        pipeline_scope.with_legacy_security_reports.first
+    end
   end
 end

ee/app/models/vulnerabilities/statistic.rb

@@ -5,6 +5,7 @@ module Vulnerabilities
     self.table_name = 'vulnerability_statistics'
 
     belongs_to :project, optional: false
+    belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :latest_pipeline_id
 
     enum letter_grade: { a: 0, b: 1, c: 2, d: 3, f: 4 }
 
@@ -37,6 +38,32 @@ module Vulnerabilities
           letter_grades[:a]
         end
       end
+
+      def set_latest_pipeline_with(pipeline)
+        upsert_sql = upsert_latest_pipeline_id_sql(pipeline)
+
+        connection.execute(upsert_sql)
+      end
+
+      private
+
+      UPSERT_LATEST_PIPELINE_ID_SQL_TEMPLATE = <<~SQL
+        INSERT INTO %<table_name>s AS target (project_id, latest_pipeline_id, letter_grade, created_at, updated_at)
+          VALUES (%{project_id}, %<latest_pipeline_id>d, %<letter_grade>d, now(), now())
+        ON CONFLICT (project_id)
+          DO UPDATE SET
+            latest_pipeline_id = %<latest_pipeline_id>d, updated_at = now()
+      SQL
+
+      private_constant :UPSERT_LATEST_PIPELINE_ID_SQL_TEMPLATE
+
+      def upsert_latest_pipeline_id_sql(pipeline)
+        format(UPSERT_LATEST_PIPELINE_ID_SQL_TEMPLATE,
+               table_name: table_name,
+               project_id: pipeline.project.id,
+               latest_pipeline_id: pipeline.id,
+               letter_grade: letter_grades[:a])
+      end
     end
 
     private

ee/app/services/security/store_reports_service.rb

@@ -12,6 +12,7 @@ module Security
     def execute
       store_reports
       mark_project_as_vulnerable!
+      set_latest_pipeline!
 
       errors.any? ? error(full_errors) : success
     end
@@ -33,6 +34,10 @@ module Security
       project.project_setting.update!(has_vulnerabilities: true)
     end
 
+    def set_latest_pipeline!
+      Vulnerabilities::Statistic.set_latest_pipeline_with(pipeline)
+    end
+
     def full_errors
       errors.join(", ")
     end

ee/spec/models/project_spec.rb

@@ -1687,16 +1687,16 @@ RSpec.describe Project do
   end
 
   describe '#latest_pipeline_with_security_reports' do
-    let(:only_successful) { false }
-
-    let_it_be(:project) { create(:project) }
+    let_it_be(:project, refind: true) { create(:project) }
     let_it_be(:pipeline_1) { create(:ci_pipeline, :success, project: project) }
     let_it_be(:pipeline_2) { create(:ci_pipeline, project: project) }
     let_it_be(:pipeline_3) { create(:ci_pipeline, :success, project: project) }
 
     subject { project.latest_pipeline_with_security_reports(only_successful: only_successful) }
 
-    context 'when all pipelines are used' do
+    shared_examples_for 'on-the-fly latest_pipeline_with_security_reports calculation' do |expected:|
+      let(:expected_pipeline) { public_send(expected) }
+
       context 'when legacy reports are used' do
         before do
           create(:ee_ci_build, :legacy_sast, pipeline: pipeline_1)
@@ -1704,7 +1704,7 @@ RSpec.describe Project do
         end
 
         it 'returns the latest pipeline with security reports' do
-          is_expected.to eq(pipeline_2)
+          is_expected.to eq(expected_pipeline)
         end
       end
 
@@ -1715,7 +1715,7 @@ RSpec.describe Project do
         end
 
         it 'returns the latest pipeline with security reports' do
-          is_expected.to eq(pipeline_2)
+          is_expected.to eq(expected_pipeline)
         end
 
         context 'when legacy used' do
@@ -1724,46 +1724,46 @@ RSpec.describe Project do
           end
 
           it 'prefers the new reports' do
-            is_expected.to eq(pipeline_2)
+            is_expected.to eq(expected_pipeline)
           end
         end
       end
     end
 
-    context 'when only successful pipelines are used' do
-      let(:only_successful) { true }
+    context 'when all pipelines are used' do
+      let(:only_successful) { false }
 
-      context 'when legacy reports are used' do
-        before do
-          create(:ee_ci_build, :legacy_sast, pipeline: pipeline_1)
-          create(:ee_ci_build, :legacy_sast, pipeline: pipeline_2)
+      context 'when there is no associated `vulnerability_statistic` record with the project' do
+        it_behaves_like 'on-the-fly latest_pipeline_with_security_reports calculation', expected: :pipeline_2
       end
 
-        it "returns the latest succesful pipeline with security reports" do
-          is_expected.to eq(pipeline_1)
+      context 'when there is an associated `vulnerability_statistic` record with the project' do
+        context 'when the pipeline of `vulnerability_statistic` has not been set' do
+          it_behaves_like 'on-the-fly latest_pipeline_with_security_reports calculation', expected: :pipeline_2 do
+            before do
+              create(:vulnerability_statistic, project: project, pipeline: nil)
+            end
           end
         end
 
-      context 'when new reports are used' do
+        context 'when the pipeline of `vulnerability_statistic` has been set' do
           before do
-          create(:ee_ci_build, :sast, pipeline: pipeline_1)
-          create(:ee_ci_build, :sast, pipeline: pipeline_2)
+            create(:vulnerability_statistic, project: project, pipeline: pipeline_1)
           end
 
-        it 'returns the latest successful pipeline with security reports' do
-          is_expected.to eq(pipeline_1)
+          it { is_expected.to eq(pipeline_1) }
+        end
+      end
     end
 
-        context 'when legacy used' do
+    context 'when only successful pipelines are used' do
+      let(:only_successful) { true }
+
       before do
-            create(:ee_ci_build, :legacy_sast, pipeline: pipeline_3)
+        create(:vulnerability_statistic, project: project, pipeline: pipeline_2)
       end
 
-          it 'prefers the new reports' do
-            is_expected.to eq(pipeline_1)
-          end
-        end
-      end
+      it_behaves_like 'on-the-fly latest_pipeline_with_security_reports calculation', expected: :pipeline_1
     end
   end
 

ee/spec/models/vulnerabilities/statistic_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe Vulnerabilities::Statistic do
   describe 'associations' do
     it { is_expected.to belong_to(:project).required(true) }
+    it { is_expected.to belong_to(:pipeline).required(false) }
   end
 
   describe 'validations' do
@@ -45,4 +46,26 @@ RSpec.describe Vulnerabilities::Statistic do
       it { is_expected.to eq(3) }
     end
   end
+
+  describe '.set_latest_pipeline_with' do
+    let_it_be(:pipeline) { create(:ci_pipeline) }
+    let_it_be(:project) { pipeline.project }
+
+    subject(:set_latest_pipeline) { described_class.set_latest_pipeline_with(pipeline) }
+
+    context 'when there is already a vulnerability_statistic record available for the project of given pipeline' do
+      let(:vulnerability_statistic) { create(:vulnerability_statistic, project: project) }
+
+      it 'updates the `latest_pipeline_id` attribute of the existing record' do
+        expect { set_latest_pipeline }.to change { vulnerability_statistic.reload.pipeline }.from(nil).to(pipeline)
+      end
+    end
+
+    context 'when there is no vulnerability_statistic record available for the project of given pipeline' do
+      it 'creates a new record with the `latest_pipeline_id` attribute is set' do
+        expect { set_latest_pipeline }.to change { project.reload.vulnerability_statistic }.from(nil).to(an_instance_of(described_class))
+                                      .and change { project.vulnerability_statistic&.pipeline }.from(nil).to(pipeline)
+      end
+    end
+  end
 end

ee/spec/services/security/store_reports_service_spec.rb

@@ -3,13 +3,13 @@
 require 'spec_helper'
 
 RSpec.describe Security::StoreReportsService do
-  let(:user) { create(:user) }
-  let(:group)   { create(:group) }
-  let(:project) { create(:project, :public, namespace: group) }
-  let(:pipeline) { create(:ci_pipeline, project: project) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, :public, namespace: group) }
+  let_it_be(:pipeline) { create(:ci_pipeline, project: project) }
 
   describe '#execute' do
-    subject { described_class.new(pipeline).execute }
+    subject(:execute_service_object) { described_class.new(pipeline).execute }
 
     context 'when there are reports' do
       before do
@@ -30,11 +30,15 @@ RSpec.describe Security::StoreReportsService do
             end
           end
 
-        subject
+        execute_service_object
       end
 
       it 'marks the project as vulnerable' do
-        expect { subject }.to change { project.project_setting.has_vulnerabilities }.from(false).to(true)
+        expect { execute_service_object }.to change { project.reload.project_setting.has_vulnerabilities }.from(false).to(true)
+      end
+
+      it 'updates the `latest_pipeline_id` attribute of the associated `vulnerability_statistic` record' do
+        expect { execute_service_object }.to change { project.reload.vulnerability_statistic&.latest_pipeline_id }.from(nil).to(pipeline.id)
       end
 
       context 'when StoreReportService returns an error for a report' do

ee/spec/services/vulnerabilities/statistics/adjustment_service_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Vulnerabilities::Statistics::AdjustmentService do
-  let_it_be(:project) { create(:project) }
+  let_it_be_with_reload(:project) { create(:project) }
 
   describe '.execute' do
     let(:project_ids) { [1, 2, 3] }
@@ -23,8 +23,7 @@ RSpec.describe Vulnerabilities::Statistics::AdjustmentService do
   end
 
   describe '#execute' do
-    let(:project) { create(:project) }
-    let(:statistics) { project.vulnerability_statistic.reload.as_json(except: [:id, :project_id, :created_at, :updated_at]) }
+    let(:statistics) { project.vulnerability_statistic.reload.as_json(only: expected_statistics.keys) }
     let(:project_ids) { [project.id] }
 
     let(:expected_statistics) do
@@ -51,7 +50,7 @@ RSpec.describe Vulnerabilities::Statistics::AdjustmentService do
       let(:project_ids) { (1..1001).to_a }
 
       it 'raises error' do
-        expect {adjust_statistics}.to raise_error(described_class::TooManyProjectsError, 'Cannot adjust statistics for more than 1000 projects')
+        expect { adjust_statistics }.to raise_error(described_class::TooManyProjectsError, 'Cannot adjust statistics for more than 1000 projects')
       end
     end