Merge branch '218012-rewrite-query-to-fetch-reports' into 'master'

Rewrite query to check resolved_on_default_branch

See merge request gitlab-org/gitlab!38452

ee/app/models/ee/project.rb

@@ -233,9 +233,12 @@ module EE
       self.tracing_setting.try(:external_url)
     end
 
-    def latest_pipeline_with_security_reports
-      all_pipelines.newest_first(ref: default_branch).with_reports(::Ci::JobArtifact.security_reports).first ||
-        all_pipelines.newest_first(ref: default_branch).with_legacy_security_reports.first
+    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
     end
 
     def latest_pipeline_with_reports(reports)

ee/app/models/vulnerability.rb

@@ -141,9 +141,18 @@ class Vulnerability < ApplicationRecord
   def resolved_on_default_branch
     return false unless findings.any?
 
-    latest_successful_pipeline_for_default_branch = project.latest_successful_pipeline_for_default_branch
-    latest_pipeline_with_vulnerability = finding.pipelines.order(created_at: :desc).first
-    latest_pipeline_with_vulnerability != latest_successful_pipeline_for_default_branch
+    # We can't just use project.latest_successful_pipeline_for_default_branch
+    # because there's no guarantee that it actually ran the security jobs
+    # See https://gitlab.com/gitlab-org/gitlab/-/issues/218012
+    latest_successful_pipeline = project
+      .latest_pipeline_with_security_reports(only_successful: true)
+
+    # Technically this shouldn't ever happen.
+    # If an vulnerability was discovered, then we must have ran a scan of the
+    # appropriate type at least once.
+    return false unless latest_successful_pipeline
+
+    finding.pipelines.exclude?(latest_successful_pipeline)
   end
 
   def user_notes_count

ee/changelogs/unreleased/218012-rewrite-query-to-fetch-reports.yml

+---
+title: Fetch latest successful pipeline with security jobs to check if vulnerability
+  was resolved
+merge_request: 38452
+author:
+type: changed

ee/spec/models/project_spec.rb

@@ -1513,20 +1513,23 @@ RSpec.describe Project do
   end
 
   describe '#latest_pipeline_with_security_reports' do
-    let(:project) { create(:project) }
-    let!(:pipeline_1) { create(:ci_pipeline, project: project) }
-    let!(:pipeline_2) { create(:ci_pipeline, project: project) }
-    let!(:pipeline_3) { create(:ci_pipeline, project: project) }
+    let(:only_successful) { false }
+
+    let_it_be(:project) { 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 }
+    subject { project.latest_pipeline_with_security_reports(only_successful: only_successful) }
 
+    context 'when all pipelines are used' do
       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)
         end
 
-      it "returns the latest pipeline with security reports" do
+        it 'returns the latest pipeline with security reports' do
           is_expected.to eq(pipeline_2)
         end
       end
@@ -1537,7 +1540,7 @@ RSpec.describe Project do
           create(:ee_ci_build, :sast, pipeline: pipeline_2)
         end
 
-      it "returns the latest pipeline with security reports" do
+        it 'returns the latest pipeline with security reports' do
           is_expected.to eq(pipeline_2)
         end
 
@@ -1546,13 +1549,50 @@ RSpec.describe Project do
             create(:ee_ci_build, :legacy_sast, pipeline: pipeline_3)
           end
 
-        it "prefers the new reports" do
+          it 'prefers the new reports' do
             is_expected.to eq(pipeline_2)
           end
         end
       end
     end
 
+    context 'when only successful pipelines are used' do
+      let(:only_successful) { true }
+
+      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)
+        end
+
+        it "returns the latest succesful pipeline with security reports" do
+          is_expected.to eq(pipeline_1)
+        end
+      end
+
+      context 'when new reports are used' do
+        before do
+          create(:ee_ci_build, :sast, pipeline: pipeline_1)
+          create(:ee_ci_build, :sast, pipeline: pipeline_2)
+        end
+
+        it 'returns the latest successful pipeline with security reports' do
+          is_expected.to eq(pipeline_1)
+        end
+
+        context 'when legacy used' do
+          before do
+            create(:ee_ci_build, :legacy_sast, pipeline: pipeline_3)
+          end
+
+          it 'prefers the new reports' do
+            is_expected.to eq(pipeline_1)
+          end
+        end
+      end
+    end
+  end
+
   describe '#latest_pipeline_with_reports' do
     let(:project) { create(:project) }
     let!(:pipeline_1) { create(:ee_ci_pipeline, :with_sast_report, project: project) }

ee/spec/models/vulnerability_spec.rb

@@ -250,7 +250,7 @@ RSpec.describe Vulnerability do
 
   describe '#resolved_on_default_branch' do
     let_it_be(:project) { create(:project, :repository, :with_vulnerability) }
-    let_it_be(:pipeline_with_vulnerability) { create(:ci_pipeline, :success, project: project, sha: project.commit.id) }
+    let_it_be(:pipeline_with_vulnerability) { create(:ee_ci_pipeline, :success, :with_sast_report, project: project, sha: project.commit.id) }
     let_it_be(:vulnerability) { project.vulnerabilities.first }
     let_it_be(:finding1) { create(:vulnerabilities_occurrence, vulnerability: vulnerability, pipelines: [pipeline_with_vulnerability]) }
     let_it_be(:finding2) { create(:vulnerabilities_occurrence, vulnerability: vulnerability, pipelines: [pipeline_with_vulnerability]) }
@@ -259,14 +259,22 @@ RSpec.describe Vulnerability do
 
     context 'Vulnerability::Finding is present on the pipeline for default branch' do
       it { is_expected.to eq(false) }
+
+      context 'but pipeline is failed' do
+        let!(:unsucessful_pipeline_with_vulnerability) { create(:ee_ci_pipeline, :with_sast_report, :failed, project: project, sha: project.commit.id) }
+
+        it { is_expected.to eq(false) }
+      end
     end
 
-    context 'Vulnerability::Finding is not present on the pipeline for default branch' do
-      before do
-        project.instance_variable_set(:@latest_successful_pipeline_for_default_branch, pipeline_without_vulnerability)
+    context 'Vulnerability::Finding is not present on the latest pipeline without security job' do
+      let!(:pipeline_without_security_job) { create(:ee_ci_pipeline, :success, project: project, sha: project.commit.id) }
+
+      it { is_expected.to eq(false) }
     end
 
-      let_it_be(:pipeline_without_vulnerability) { create(:ci_pipeline, :success, project: project, sha: project.commit.id) }
+    context 'Vulnerability::Finding is not present on the pipeline for default branch' do
+      let!(:pipeline_without_vulnerability) { create(:ee_ci_pipeline, :success, :with_sast_report, project: project, sha: project.commit.id) }
 
       it { is_expected.to eq(true) }
     end