Merge branch 'bwill/reduce-sbom-occurrences-table-writes' into 'master'

Reduce sbom_occurrences table writes

See merge request !148868



Merged-by: Ash McKenzie <amckenzie@gitlab.com>
Approved-by: Ash McKenzie <amckenzie@gitlab.com>
Reviewed-by: Ash McKenzie <amckenzie@gitlab.com>
Reviewed-by: Brian Williams <bwilliams@gitlab.com>
Co-authored-by: Brian Williams <bwilliams@gitlab.com>

ee/app/models/sbom/occurrence.rb

@@ -79,6 +79,7 @@ class Occurrence < ApplicationRecord
     end
 
     scope :by_project_ids, ->(project_ids) { where(project_id: project_ids) }
+    scope :by_uuids, ->(uuids) { where(uuid: uuids) }
 
     scope :filter_by_package_managers, ->(package_managers) do
       where(package_manager: package_managers)

ee/app/services/sbom/ingestion/occurrence_map.rb

@@ -6,7 +6,7 @@ class OccurrenceMap
       include Gitlab::Utils::StrongMemoize
 
       attr_reader :report_component, :report_source, :vulnerabilities
-      attr_accessor :component_id, :component_version_id, :source_id, :occurrence_id, :source_package_id
+      attr_accessor :component_id, :component_version_id, :source_id, :occurrence_id, :source_package_id, :uuid
 
       def initialize(report_component, report_source, vulnerabilities)
         @report_component = report_component
@@ -25,6 +25,7 @@ def to_h
           source: report_source&.data,
           source_package_id: source_package_id,
           source_package_name: report_component.source_package_name,
+          uuid: uuid,
           version: version
         }
       end

ee/app/services/sbom/ingestion/tasks/ingest_occurrences.rb

@@ -16,12 +16,19 @@ def after_ingest
           each_pair do |occurrence_map, row|
             occurrence_map.occurrence_id = row.first
           end
+
+          existing_occurrences_by_uuid.each_pair do |uuid, occurrence|
+            indexed_occurrence_maps[[uuid]].each { |map| map.occurrence_id = occurrence.id }
+          end
         end
 
         def attributes
-          occurrence_maps.uniq! { |occurrence_map| uuid(occurrence_map) }
-          occurrence_maps.map do |occurrence_map|
-            {
+          ensure_uuids
+          occurrence_maps.uniq!(&:uuid)
+          occurrence_maps.filter_map do |occurrence_map|
+            uuid = occurrence_map.uuid
+
+            new_attributes = {
               project_id: project.id,
               pipeline_id: pipeline.id,
               component_id: occurrence_map.component_id,
@@ -29,7 +36,7 @@ def attributes
               source_id: occurrence_map.source_id,
               source_package_id: occurrence_map.source_package_id,
               commit_sha: pipeline.sha,
-              uuid: uuid(occurrence_map),
+              uuid: uuid,
               package_manager: occurrence_map.packager,
               input_file_path: occurrence_map.input_file_path,
               licenses: licenses.fetch(occurrence_map.report_component, []),
@@ -44,8 +51,35 @@ def attributes
                 attrs.except!(:vulnerability_count, :highest_severity)
               end
             end
+
+            existing_occurrence = existing_occurrences_by_uuid[uuid]
+            existing_attributes = existing_occurrence&.attributes&.symbolize_keys&.slice(*new_attributes.keys)
+
+            if new_attributes != existing_attributes
+              # Remove updated items from the list so that we don't have to iterate over them
+              # twice when setting the ids in `after_ingest`.
+              existing_occurrences_by_uuid.delete(uuid)
+
+              new_attributes
+            end
+          end
+        end
+
+        def uuids
+          occurrence_maps.map do |map|
+            map.uuid = uuid(map)
           end
         end
+        strong_memoize_attr :uuids
+
+        alias_method :ensure_uuids, :uuids
+
+        def existing_occurrences_by_uuid
+          return {} unless uuids.present?
+
+          Sbom::Occurrence.by_uuids(uuids).index_by(&:uuid)
+        end
+        strong_memoize_attr :existing_occurrences_by_uuid
 
         def uuid(occurrence_map)
           uuid_attributes = occurrence_map.to_h.slice(
@@ -57,10 +91,6 @@ def uuid(occurrence_map)
           ::Sbom::OccurrenceUUID.generate(**uuid_attributes)
         end
 
-        def grouping_key_for_map(map)
-          [uuid(map)]
-        end
-
         def licenses
           Licenses.new(project, occurrence_maps)
         end

ee/spec/factories/sbom/occurrences.rb

@@ -8,6 +8,7 @@
     component_version { association :sbom_component_version }
     component { component_version&.component || association(:sbom_component) }
     source { association :sbom_source, packager_name: packager_name }
+    source_package { association :sbom_source_package }
 
     trait :os_occurrence do
       source do
@@ -78,9 +79,9 @@
         source_id: occurrence.source&.id
       )
 
-      occurrence.package_manager = occurrence.source&.source&.dig('package_manager', 'name')
-      occurrence.input_file_path = occurrence.source&.source&.dig('input_file', 'path')
-      occurrence.component_name = occurrence.component&.name
+      occurrence.package_manager ||= occurrence.source&.source&.dig('package_manager', 'name')
+      occurrence.input_file_path ||= occurrence.source&.source&.dig('input_file', 'path')
+      occurrence.component_name ||= occurrence.component&.name
     end
   end
 end

ee/spec/models/sbom/occurrence_spec.rb

@@ -315,6 +315,12 @@
     end
   end
 
+  describe '.by_uuids' do
+    let_it_be(:occurrences) { create_list(:sbom_occurrence, 2) }
+
+    specify { expect(described_class.by_uuids(occurrences.first.uuid)).to eq([occurrences.first]) }
+  end
+
   describe '.filter_by_package_managers' do
     let_it_be(:occurrence_nuget) { create(:sbom_occurrence, packager_name: 'nuget') }
     let_it_be(:occurrence_npm) { create(:sbom_occurrence, packager_name: 'npm') }

ee/spec/services/sbom/ingestion/occurrence_map_spec.rb

@@ -17,6 +17,7 @@
       source: report_source.data,
       source_id: nil,
       source_type: report_source.source_type,
+      uuid: nil,
       source_package_id: nil,
       source_package_name: report_component.source_package_name,
       version: report_component.version
@@ -66,6 +67,7 @@
             source: nil,
             source_id: nil,
             source_type: nil,
+            uuid: nil,
             source_package_id: nil,
             source_package_name: report_component.source_package_name,
             version: report_component.version
@@ -88,6 +90,7 @@
             source: report_source.data,
             source_id: nil,
             source_type: report_source.source_type,
+            uuid: nil,
             source_package_id: nil,
             source_package_name: report_component.source_package_name,
             version: report_component.version
@@ -113,6 +116,7 @@
             source: report_source.data,
             source_id: nil,
             source_type: report_source.source_type,
+            uuid: nil,
             source_package_id: nil,
             source_package_name: report_component.source_package_name,
             version: report_component.version

ee/spec/services/sbom/ingestion/tasks/ingest_occurrences_spec.rb

@@ -4,7 +4,7 @@
 
 RSpec.describe Sbom::Ingestion::Tasks::IngestOccurrences, feature_category: :dependency_management do
   describe '#execute' do
-    let_it_be(:pipeline) { build(:ci_pipeline) }
+    let_it_be(:pipeline) { create(:ci_pipeline) }
 
     let(:project) { pipeline.project }
     let(:occurrence_maps) { create_list(:sbom_occurrence_map, 4, :for_occurrence_ingestion) }
@@ -145,18 +145,54 @@
 
     context 'when there is an existing occurrence' do
       let!(:existing_occurrence) do
-        attributes = occurrence_maps.first.to_h.slice(
-          :component_id,
-          :component_version_id,
-          :source_id
-        )
-
-        create(:sbom_occurrence, pipeline: pipeline, **attributes)
+        occurrence_map = occurrence_maps.first
+        attributes = {
+          project_id: project.id,
+          pipeline_id: pipeline.id,
+          component_id: occurrence_map.component_id,
+          component_version_id: occurrence_map.component_version_id,
+          source_id: occurrence_map.source_id,
+          source_package_id: occurrence_map.source_package_id,
+          commit_sha: pipeline.sha,
+          licenses: [],
+          component_name: occurrence_map.name,
+          input_file_path: occurrence_map.input_file_path,
+          highest_severity: occurrence_map.highest_severity,
+          vulnerability_count: occurrence_map.vulnerability_count,
+          traversal_ids: project.namespace.traversal_ids,
+          archived: project.archived,
+          ancestors: occurrence_map.ancestors
+        }
+
+        create(:sbom_occurrence, **attributes)
       end
 
       it 'does not create a new record for the existing version' do
         expect { ingest_occurrences }.to change(Sbom::Occurrence, :count).by(3)
-        expect(occurrence_maps).to all(have_attributes(occurrence_id: Integer))
+        expect(occurrence_maps.map(&:occurrence_id)).to match_array([Integer, Integer, Integer, existing_occurrence.id])
+      end
+
+      it 'does not perform database writes for existing records' do
+        recorder = ActiveRecord::QueryRecorder.new { ingest_occurrences }
+
+        inserts = recorder.occurrences_starting_with("INSERT INTO")
+        expect(inserts.size).to eq(1)
+        sql = inserts.first.first
+        expect(sql).not_to include(existing_occurrence.uuid)
+      end
+
+      context 'when an attribute has been changed' do
+        let_it_be(:other_project) { create(:project) }
+
+        before do
+          existing_occurrence.update!(project: other_project, traversal_ids: other_project.namespace.traversal_ids)
+        end
+
+        it 'updates the record' do
+          expect { ingest_occurrences }.to change { existing_occurrence.reload.project }.from(other_project).to(project)
+            .and change { existing_occurrence.reload.traversal_ids }
+            .from(other_project.namespace.traversal_ids).to(project.namespace.traversal_ids)
+        end
       end
     end