Move the logic of finding uuid calculation

We are moving this logic from `StoreReportService` to parsing step as
we will need the `uuid` values not only after we store the findings.

ee/app/models/ee/ci/job_artifact.rb

@@ -118,7 +118,7 @@ module EE
       strong_memoize(:security_report) do
         next unless file_type.in?(SECURITY_REPORT_FILE_TYPES)
 
-        report = ::Gitlab::Ci::Reports::Security::Report.new(file_type, nil, nil).tap do |report|
+        report = ::Gitlab::Ci::Reports::Security::Report.new(file_type, job.pipeline, nil).tap do |report|
           each_blob do |blob|
             ::Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, report)
           end

ee/app/models/security/finding.rb

@@ -38,7 +38,7 @@ module Security
                 .where('vulnerability_feedback.project_fingerprint = security_findings.project_fingerprint'))
     end
     scope :ordered, -> { order(severity: :desc, confidence: :desc, id: :asc) }
-    scope :with_build_and_artifacts, -> { includes(build: :job_artifacts) }
+    scope :with_build_and_artifacts, -> { includes(build: [:job_artifacts, :pipeline]) }
     scope :with_scan, -> { includes(:scan) }
     scope :with_scanner, -> { includes(:scanner) }
     scope :deduplicated, -> { where(deduplicated: true) }

ee/app/services/security/store_report_service.rb

@@ -48,7 +48,6 @@ module Security
       end
 
       vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :scan, :links)
-      vulnerability_params[:uuid] = calculate_uuid_v5(finding)
       vulnerability_finding = create_or_find_vulnerability_finding(finding, vulnerability_params)
 
       update_vulnerability_scanner(finding)
@@ -92,23 +91,6 @@ module Security
       end
     end
 
-    def calculate_uuid_v5(vulnerability_finding)
-      uuid_v5_name_components = {
-        report_type: vulnerability_finding.report_type,
-        primary_identifier_fingerprint: vulnerability_finding.primary_fingerprint,
-        location_fingerprint: vulnerability_finding.location.fingerprint,
-        project_id: project.id
-      }
-
-      if uuid_v5_name_components.values.any?(&:nil?)
-        Gitlab::AppLogger.warn(message: "One or more UUID name components are nil", components: uuid_v5_name_components)
-      end
-
-      name = uuid_v5_name_components.values.join('-')
-
-      Gitlab::Vulnerabilities::CalculateFindingUUID.call(name)
-    end
-
     def update_vulnerability_scanner(finding)
       scanner = scanners_objects[finding.scanner.key]
       scanner.update!(finding.scanner.to_hash)

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

@@ -61,7 +61,7 @@ module Gitlab
 
             report.add_finding(
               ::Gitlab::Ci::Reports::Security::Finding.new(
-                uuid: SecureRandom.uuid,
+                uuid: calculate_uuid_v5(report, location, identifiers.first),
                 report_type: report.type,
                 name: finding_name(data, identifiers, location),
                 compare_key: data['cve'] || '',
@@ -160,6 +160,23 @@ module Gitlab
             identifier = identifiers.find(&:cve?) || identifiers.find(&:cwe?) || identifiers.first
             "#{identifier.name} in #{location&.fingerprint_path}"
           end
+
+          def calculate_uuid_v5(report, location, primary_identifier)
+            uuid_v5_name_components = {
+              report_type: report.type,
+              primary_identifier_fingerprint: primary_identifier&.fingerprint,
+              location_fingerprint: location&.fingerprint,
+              project_id: report.project_id
+            }
+
+            if uuid_v5_name_components.values.any?(&:nil?)
+              Gitlab::AppLogger.warn(message: "One or more UUID name components are nil", components: uuid_v5_name_components)
+            end
+
+            name = uuid_v5_name_components.values.join('-')
+
+            Gitlab::Vulnerabilities::CalculateFindingUUID.call(name)
+          end
         end
       end
     end

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

@@ -8,9 +8,6 @@ module Gitlab
           attr_reader :created_at, :type, :pipeline, :findings, :scanners, :identifiers
           attr_accessor :scan, :scanned_resources, :error
 
-          delegate :project, to: :pipeline
-          delegate :id, to: :project, prefix: true
-
           def initialize(type, pipeline, created_at)
             @type = type
             @pipeline = pipeline
@@ -58,6 +55,15 @@ module Gitlab
           def primary_scanner
             scanners.first&.second
           end
+
+          # It's important to read the `project_id` attribute instead of calling
+          # `project_id` on pipeline. Because the `Ci::Pipeline` delegates the `project_id`
+          # call to the `project` relation even though it already has the attribute called
+          # `project_id`. By reading attribute directly from the entity, we are preventing
+          # an extra database query to load the project.
+          def project_id
+            pipeline&.read_attribute(:project_id)
+          end
         end
       end
     end

ee/lib/gitlab/vulnerabilities/calculate_finding_uuid.rb

@@ -7,7 +7,7 @@ module Gitlab
         development: "a143e9e2-41b3-47bc-9a19-081d089229f4",
         test: "a143e9e2-41b3-47bc-9a19-081d089229f4",
         staging: "a6930898-a1b2-4365-ab18-12aa474d9b26",
-        production:  "58dc0f06-936c-43b3-93bb-71693f1b6570"
+        production: "58dc0f06-936c-43b3-93bb-71693f1b6570"
       }.freeze
 
       NAMESPACE_REGEX = /(\h{8})-(\h{4})-(\h{4})-(\h{4})-(\h{4})(\h{8})/.freeze
@@ -18,10 +18,12 @@ module Gitlab
       end
 
       def self.namespace_id
-        namespace_uuid = FINDING_NAMESPACES_IDS.fetch(Rails.env.to_sym)
-        # Digest::UUID is broken when using an UUID in namespace_id
-        # https://github.com/rails/rails/issues/37681#issue-520718028
-        namespace_uuid.scan(NAMESPACE_REGEX).flatten.map { |s| s.to_i(16) }.pack(PACK_PATTERN)
+        @namespace_id ||= begin
+          namespace_uuid = FINDING_NAMESPACES_IDS.fetch(Rails.env.to_sym)
+          # Digest::UUID is broken when using a UUID as a namespace_id
+          # https://github.com/rails/rails/issues/37681#issue-520718028
+          namespace_uuid.scan(NAMESPACE_REGEX).flatten.map { |s| s.to_i(16) }.pack(PACK_PATTERN)
+        end
       end
     end
   end

ee/spec/finders/security/findings_finder_spec.rb

@@ -91,7 +91,7 @@ RSpec.describe Security::FindingsFinder do
         end
 
         it 'does not cause N+1 queries' do
-          expect { finder_result }.not_to exceed_query_limit(7)
+          expect { finder_result }.not_to exceed_query_limit(8)
         end
 
         describe '#current_page' do

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

@@ -163,5 +163,20 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
         expect(links.first).to be_a(::Gitlab::Ci::Reports::Security::Link)
       end
     end
+
+    describe 'setting the uuid' do
+      let(:finding_uuids) { report.findings.map(&:uuid) }
+      let(:expected_uuids) do
+        [
+          Gitlab::Vulnerabilities::CalculateFindingUUID.call("dependency_scanning--33dc9f32c77dde16d39c69d3f78f27ca3114a7c5-#{pipeline.project_id}"),
+          Gitlab::Vulnerabilities::CalculateFindingUUID.call("dependency_scanning--33dc9f32c77dde16d39c69d3f78f27ca3114a7c5-#{pipeline.project_id}"),
+          Gitlab::Vulnerabilities::CalculateFindingUUID.call("dependency_scanning--33dc9f32c77dde16d39c69d3f78f27ca3114a7c5-#{pipeline.project_id}")
+        ]
+      end
+
+      it 'sets the UUIDv5 for findings' do
+        expect(finding_uuids).to match_array(expected_uuids)
+      end
+    end
   end
 end

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

@@ -6,12 +6,9 @@ RSpec.describe Gitlab::Ci::Reports::Security::Report do
   let_it_be(:pipeline) { create(:ci_pipeline) }
 
   let(:created_at) { 2.weeks.ago }
-
-  subject(:report) { described_class.new('sast', pipeline, created_at) }
+  let(:report) { described_class.new('sast', pipeline, created_at) }
 
   it { expect(report.type).to eq('sast') }
-  it { is_expected.to delegate_method(:project).to(:pipeline) }
-  it { is_expected.to delegate_method(:id).to(:project).with_prefix }
 
   describe '#add_scanner' do
     let(:scanner) { create(:ci_reports_security_scanner, external_id: 'find_sec_bugs') }
@@ -140,4 +137,10 @@ RSpec.describe Gitlab::Ci::Reports::Security::Report do
 
     it { is_expected.to eq(scanner_1) }
   end
+
+  describe '#project_id' do
+    subject { report.project_id }
+
+    it { is_expected.to eq(pipeline.project_id) }
+  end
 end

ee/spec/lib/gitlab/vulnerabilities/calculate_finding_uuid_spec.rb

@@ -7,6 +7,11 @@ RSpec.describe Gitlab::Vulnerabilities::CalculateFindingUUID do
 
   subject { described_class.call(value) }
 
+  before do
+    # This is necessary to clear memoization for testing different environments
+    described_class.instance_variable_set(:@namespace_id, nil)
+  end
+
   context 'in development' do
     let_it_be(:development_proper_uuid) { "5b593e54-90f5-504b-8805-5394a4d14b94" }
 

ee/spec/services/security/store_report_service_spec.rb

@@ -2,19 +2,6 @@
 
 require 'spec_helper'
 
-UUID_REGEXP = Regexp.new("^([0-9a-f]{8})-([0-9a-f]{4})-([0-9a-f]{4})-" \
-                         "([0-9a-f]{2})([0-9a-f]{2})-([0-9a-f]{12})$").freeze
-
-RSpec::Matchers.define :be_uuid_v5 do
-  match do |string|
-    expect(string).to be_a(String)
-
-    uuid_components = string.downcase.scan(UUID_REGEXP).first
-    time_hi_and_version = uuid_components[2].to_i(16)
-    (time_hi_and_version >> 12) == 5
-  end
-end
-
 RSpec.describe Security::StoreReportService, '#execute' do
   let_it_be(:user) { create(:user) }
   let(:artifact) { create(:ee_ci_job_artifact, trait) }
@@ -77,12 +64,6 @@ RSpec.describe Security::StoreReportService, '#execute' do
       it 'inserts all vulnerabilties' do
         expect { subject }.to change { Vulnerability.count }.by(findings)
       end
-
-      it 'calculates UUIDv5 for all findings' do
-        subject
-        uuids = Vulnerabilities::Finding.pluck(:uuid)
-        expect(uuids).to all(be_uuid_v5)
-      end
     end
 
     context 'invalid data' do
@@ -148,10 +129,6 @@ RSpec.describe Security::StoreReportService, '#execute' do
       expect { subject }.to change { Vulnerabilities::Finding.count }.by(32)
     end
 
-    it 'calculates UUIDv5 for all findings' do
-      expect(Vulnerabilities::Finding.pluck(:uuid)).to all(be_a(String))
-    end
-
     it 'inserts all finding pipelines (join model) for this new pipeline' do
       expect { subject }.to change { Vulnerabilities::FindingPipeline.where(pipeline: new_pipeline).count }.by(33)
     end