Merge branch 'remove_scanned_resources_from_mr_api_231424' into 'master'

Remove scanned_resources_count from security scan

See merge request gitlab-org/gitlab!38761

ee/app/models/ee/ci/build.rb

@@ -39,10 +39,6 @@ module EE
             .for_ref(ref)
             .for_project_paths(project_path)
         end
-
-        scope :security_scans_scanned_resources_count, -> (report_types) do
-          joins(:security_scans).where(security_scans: { scan_type: report_types }).group(:scan_type).sum(:scanned_resources_count)
-        end
       end
 
       def shared_runners_minutes_limit_enabled?

ee/app/serializers/vulnerabilities/finding_reports_comparer_entity.rb

@@ -9,5 +9,4 @@ class Vulnerabilities::FindingReportsComparerEntity < Grape::Entity
   expose :added, using: Vulnerabilities::FindingEntity
   expose :fixed, using: Vulnerabilities::FindingEntity
   expose :existing, using: Vulnerabilities::FindingEntity
-  expose :scans, using: Vulnerabilities::ScanEntity
 end

ee/app/serializers/vulnerabilities/scan_entity.rb

-# frozen_string_literal: true
-
-class Vulnerabilities::ScanEntity < Grape::Entity
-  include RequestAwareEntity
-
-  expose :scanned_resources_count do |scan|
-    scan.scanned_resources_count || 0
-  end
-  expose :job_path do |scan|
-    project_job_path(scan.build.project, scan.build)
-  end
-end

ee/app/services/ci/compare_security_reports_service.rb

@@ -15,8 +15,7 @@ module Ci
     end
 
     def build_comparer(base_pipeline, head_pipeline)
-      head_scans = head_pipeline&.security_scans || Security::Scan.none
-      comparer_class.new(get_report(base_pipeline), get_report(head_pipeline), head_security_scans: head_scans)
+      comparer_class.new(get_report(base_pipeline), get_report(head_pipeline))
     end
   end
 end

ee/app/services/security/store_scans_service.rb

@@ -11,17 +11,12 @@ module Security
 
       security_reports = @build.job_artifacts.security_reports
 
-      scan_params = security_reports.map do |job_artifact|
-        {
-          build: @build,
-          scan_type: job_artifact.file_type,
-          scanned_resources_count: Gitlab::Ci::Parsers::Security::ScannedResources.new.scanned_resources_count(job_artifact)
-        }
-      end
-
       ActiveRecord::Base.transaction do
-        scan_params.each do |param|
-          Security::Scan.safe_find_or_create_by!(param)
+        security_reports.each do |report|
+          Security::Scan.safe_find_or_create_by!(
+            build: @build,
+            scan_type: report.file_type
+          )
         end
       end
     end

ee/lib/gitlab/ci/parsers/security/scanned_resources.rb

-# frozen_string_literal: true
-module Gitlab
-  module Ci
-    module Parsers
-      module Security
-        class ScannedResources
-          def scanned_resources_count(job_artifact)
-            scanned_resources_sum = 0
-            job_artifact.each_blob do |blob|
-              report_data = parse_report_json(blob)
-              scanned_resources_sum += report_data.fetch('scan', {}).fetch('scanned_resources', []).length
-            end
-            scanned_resources_sum
-          end
-
-          private
-
-          def parse_report_json(blob)
-            Gitlab::Json.parse!(blob)
-          rescue JSON::ParserError
-            {}
-          end
-        end
-      end
-    end
-  end
-end

ee/lib/gitlab/ci/reports/security/vulnerability_reports_comparer.rb

@@ -7,14 +7,13 @@ module Gitlab
         class VulnerabilityReportsComparer
           include Gitlab::Utils::StrongMemoize
 
-          attr_reader :base_report, :head_report, :security_scans
+          attr_reader :base_report, :head_report
 
           ACCEPTABLE_REPORT_AGE = 1.week
 
-          def initialize(base_report, head_report, **extra_metadata)
+          def initialize(base_report, head_report)
             @base_report = base_report
             @head_report = head_report
-            @security_scans = extra_metadata[:head_security_scans]
           end
 
           def base_report_created_at
@@ -49,10 +48,6 @@ module Gitlab
               head_report.findings & base_report.findings
             end
           end
-
-          def scans
-            @security_scans
-          end
         end
       end
     end

ee/spec/lib/gitlab/ci/parsers/security/scanned_resources_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Ci::Parsers::Security::ScannedResources do
-  let(:parser) { described_class.new }
-
-  describe '#scanned_resources_count' do
-    subject { parser.scanned_resources_count(artifact) }
-
-    context 'there are scanned resources' do
-      let(:artifact) { create(:ee_ci_job_artifact, :dast) }
-
-      it { is_expected.to be(6) }
-    end
-
-    context 'the scan key is missing' do
-      let(:artifact) { create(:ee_ci_job_artifact, :dast_missing_scan_field) }
-
-      it { is_expected.to be(0) }
-    end
-
-    context 'the scanned_resources key is missing' do
-      let(:artifact) { create(:ee_ci_job_artifact, :dast_missing_scanned_resources_field) }
-
-      it { is_expected.to be(0) }
-    end
-
-    context 'the json is invalid' do
-      let(:artifact) { create(:ee_ci_job_artifact, :dast_with_corrupted_data) }
-
-      it { is_expected.to be(0) }
-    end
-  end
-end

ee/spec/lib/gitlab/ci/reports/security/vulnerability_reports_comparer_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::VulnerabilityReportsComparer do
     allow(head_vulnerability).to receive(:location).and_return({})
   end
 
-  subject { described_class.new(base_report, head_report, head_security_scans: []) }
+  subject { described_class.new(base_report, head_report) }
 
   describe '#base_report_out_of_date' do
     context 'no base report' do
@@ -140,7 +140,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::VulnerabilityReportsComparer do
     let(:empty_report) { build(:ci_reports_security_aggregated_reports, reports: [], findings: [])}
 
     it 'returns empty array when reports are not present' do
-      comparer = described_class.new(empty_report, empty_report, head_security_scans: [])
+      comparer = described_class.new(empty_report, empty_report)
 
       expect(comparer.existing).to eq([])
       expect(comparer.fixed).to eq([])
@@ -148,7 +148,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::VulnerabilityReportsComparer do
     end
 
     it 'returns added vulnerability when base is empty and head is not empty' do
-      comparer = described_class.new(empty_report, head_report, head_security_scans: [])
+      comparer = described_class.new(empty_report, head_report)
 
       expect(comparer.existing).to eq([])
       expect(comparer.fixed).to eq([])
@@ -156,21 +156,11 @@ RSpec.describe Gitlab::Ci::Reports::Security::VulnerabilityReportsComparer do
     end
 
     it 'returns fixed vulnerability when head is empty and base is not empty' do
-      comparer = described_class.new(base_report, empty_report, head_security_scans: [])
+      comparer = described_class.new(base_report, empty_report)
 
       expect(comparer.existing).to eq([])
       expect(comparer.fixed).to eq([base_vulnerability])
       expect(comparer.added).to eq([])
     end
   end
-
-  describe 'security_scans' do
-    let(:scan) { build(:security_scan, scanned_resources_count: 10) }
-    let(:security_scans) { [scan] }
-
-    it 'sets the security scans' do
-      comparer = described_class.new(base_report, head_report, head_security_scans: security_scans)
-      expect(comparer.security_scans).to be(security_scans)
-    end
-  end
 end

ee/spec/serializers/vulnerabilities/finding_reports_comparer_entity_spec.rb

@@ -14,10 +14,7 @@ RSpec.describe Vulnerabilities::FindingReportsComparerEntity do
     let(:head_combined_reports) { build_list(:ci_reports_security_report, 1, created_at: 2.days.ago) }
     let(:head_report) { build(:ci_reports_security_aggregated_reports, reports: head_combined_reports, findings: head_findings)}
 
-    let(:scan) { create(:security_scan, scanned_resources_count: 10) }
-    let(:security_scans) { [scan] }
-
-    let(:comparer) { Gitlab::Ci::Reports::Security::VulnerabilityReportsComparer.new(base_report, head_report, head_security_scans: security_scans) }
+    let(:comparer) { Gitlab::Ci::Reports::Security::VulnerabilityReportsComparer.new(base_report, head_report) }
 
     let(:request) { double('request') }
 
@@ -34,17 +31,6 @@ RSpec.describe Vulnerabilities::FindingReportsComparerEntity do
         allow(request).to receive(:current_user).and_return(user)
       end
 
-      it 'avoids N+1 database queries' do
-        comparer = Gitlab::Ci::Reports::Security::VulnerabilityReportsComparer.new(base_report, head_report, head_security_scans: [])
-        entity = described_class.new(comparer, request: request)
-        control_count = ActiveRecord::QueryRecorder.new { entity.as_json }.count
-
-        scans = create_list(:security_scan, 5)
-        comparer = Gitlab::Ci::Reports::Security::VulnerabilityReportsComparer.new(base_report, head_report, head_security_scans: scans)
-        entity = described_class.new(comparer, request: request)
-        expect { entity.as_json }.not_to exceed_query_limit(control_count)
-      end
-
       it 'contains the added existing and fixed vulnerabilities for container scanning' do
         expect(subject.keys).to include(:added)
         expect(subject.keys).to include(:existing)
@@ -56,23 +42,6 @@ RSpec.describe Vulnerabilities::FindingReportsComparerEntity do
         expect(subject.keys).to include(:base_report_out_of_date)
         expect(subject.keys).to include(:head_report_created_at)
       end
-
-      it 'contains the scan fields' do
-        expect(subject.keys).to include(:scans)
-        expect(subject[:scans].length).to be(1)
-        expect(subject[:scans].first[:scanned_resources_count]).to be(10)
-        project = scan.build.project
-        expect(subject[:scans].first[:job_path]).to eq("/#{project.namespace.path}/#{project.path}/-/jobs/#{scan.build.id}")
-      end
-
-      context 'scanned_resources_count is nil' do
-        let(:scan) { create(:security_scan, scanned_resources_count: nil) }
-        let(:security_scans) { [scan] }
-
-        it 'shows the scanned_resources_count is 0' do
-          expect(subject[:scans].first[:scanned_resources_count]).to be(0)
-        end
-      end
     end
   end
 end

ee/spec/serializers/vulnerabilities/scan_entity_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Vulnerabilities::ScanEntity do
-  let(:scan) { build(:security_scan, scanned_resources_count: 10) }
-  let(:request) { double('request') }
-
-  let(:entity) do
-    described_class.represent(scan, request: request)
-  end
-
-  describe '#as_json' do
-    subject { entity.as_json }
-
-    it 'contains required fields' do
-      expect(subject).to include(:scanned_resources_count)
-      expect(subject).to include(:job_path)
-    end
-
-    describe 'job_path' do
-      it 'returns path to the job log' do
-        project = scan.build.project
-        expect(subject[:job_path]).to eq("/#{project.namespace.path}/#{project.path}/-/jobs/#{scan.build.id}")
-      end
-    end
-
-    describe 'scanned_resources_count' do
-      context 'is nil' do
-        let(:scan) { build(:security_scan, scanned_resources_count: nil) }
-
-        it 'shows a count of 0' do
-          expect(subject[:scanned_resources_count]).to be(0)
-        end
-      end
-
-      context 'has a value' do
-        it 'shows the count' do
-          expect(subject[:scanned_resources_count]).to be(10)
-        end
-      end
-    end
-  end
-end

ee/spec/services/ci/compare_security_reports_service_spec.rb

@@ -90,7 +90,6 @@ RSpec.describe Ci::CompareSecurityReportsService do
         let(:base_pipeline) { create(:ee_ci_pipeline) }
 
         specify { expect { subject }.not_to raise_error }
-        specify { expect(subject.scans).to be_empty }
       end
     end
   end

ee/spec/services/security/store_scans_service_spec.rb

@@ -22,22 +22,12 @@ RSpec.describe Security::StoreScansService do
       expect(scans.sast.count).to be(1)
       expect(scans.dast.count).to be(1)
     end
-
-    it 'stores the scanned resources count on the scan' do
-      subject
-
-      sast_scan = Security::Scan.sast.find_by(build: build)
-      expect(sast_scan.scanned_resources_count).to be(0)
-
-      dast_scan = Security::Scan.dast.find_by(build: build)
-      expect(dast_scan.scanned_resources_count).to be(6)
-    end
   end
 
   context 'scan already exists' do
     before do
       create(:ee_ci_job_artifact, :dast, job: build)
-      create(:security_scan, build: build, scan_type: 'dast', scanned_resources_count: 6)
+      create(:security_scan, build: build, scan_type: 'dast')
     end
 
     it 'does not save' do
@@ -46,16 +36,4 @@ RSpec.describe Security::StoreScansService do
       expect(Security::Scan.where(build: build).count).to be(1)
     end
   end
-
-  context 'artifact file does not exist' do
-    before do
-      create(:ee_ci_job_artifact, :dast_with_missing_file, job: build)
-    end
-    it 'stores 0 scanned resources on the scan' do
-      subject
-
-      scans = Security::Scan.where(build: build)
-      expect(scans.dast.first.scanned_resources_count).to be(0)
-    end
-  end
 end