Merge branch '478466-fix-updated-at' into 'master'

Refactor IngestReportsService with execute strategy

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/162588



Merged-by: Mark Chao <mchao@gitlab.com>
Approved-by: Fabien Catteau <fcatteau@gitlab.com>
Approved-by: Mark Chao <mchao@gitlab.com>
Reviewed-by: Fabien Catteau <fcatteau@gitlab.com>
Reviewed-by: Aditya Tiwari <atiwari@gitlab.com>
Co-authored-by: atiwari71 <atiwari@gitlab.com>

ee/app/services/sbom/ingestion/execution_strategy/container_scanning_for_registry.rb

+# frozen_string_literal: true
+
+module Sbom
+  module Ingestion
+    module ExecutionStrategy
+      class ContainerScanningForRegistry < Default
+        def execute
+          ingest_reports
+
+          publish_ingested_sbom_event
+        end
+      end
+    end
+  end
+end

ee/app/services/sbom/ingestion/execution_strategy/default.rb

+# frozen_string_literal: true
+
+module Sbom
+  module Ingestion
+    module ExecutionStrategy
+      class Default
+        include Gitlab::Utils::StrongMemoize
+
+        attr_reader :reports, :project, :pipeline
+
+        def initialize(reports, project, pipeline)
+          @reports = reports
+          @project = project
+          @pipeline = pipeline
+        end
+
+        def execute
+          ingest_reports
+
+          set_latest_ingested_sbom_pipeline_id
+
+          delete_not_present_occurrences
+
+          publish_ingested_sbom_event
+        end
+
+        private
+
+        attr_reader :ingested_ids
+
+        def ingest_reports
+          @ingested_ids = reports.flat_map { |report| ingest_report(report) }
+        end
+
+        def ingest_report(sbom_report)
+          IngestReportService.execute(pipeline, sbom_report, vulnerabilities_info)
+        end
+
+        def set_latest_ingested_sbom_pipeline_id
+          project.set_latest_ingested_sbom_pipeline_id(pipeline.id)
+        end
+
+        def delete_not_present_occurrences
+          DeleteNotPresentOccurrencesService.execute(pipeline, ingested_ids)
+        end
+
+        def publish_ingested_sbom_event
+          return unless ingested_ids.present? && Feature.enabled?(:dependency_scanning_using_sbom_reports, project)
+
+          Gitlab::EventStore.publish(
+            Sbom::SbomIngestedEvent.new(data: { pipeline_id: pipeline.id })
+          )
+        end
+
+        def vulnerabilities_info
+          return {} if ::Feature.enabled?(:deprecate_vulnerability_occurrence_pipelines, project)
+
+          @vulnerabilities_info ||= Sbom::Ingestion::Vulnerabilities.new(pipeline)
+        end
+      end
+    end
+  end
+end

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

@@ -25,46 +25,51 @@ def initialize(pipeline)
 
       def execute
         in_lock(lease_key, ttl: LEASE_TTL, sleep_sec: LEASE_TRY_AFTER) do
-          ingest_reports.then do |ingested_ids|
-            delete_not_present_occurrences(ingested_ids) unless container_scanning_for_registry_pipeline?
+          strategy_class.new(valid_sbom_reports, project, pipeline).execute
 
-            if ingested_ids.present? && Feature.enabled?(:dependency_scanning_using_sbom_reports, project)
-              publish_ingested_sbom_event
-            end
-          end
-
-          project.set_latest_ingested_sbom_pipeline_id(pipeline.id)
-
-          break unless Feature.enabled?(:store_sbom_report_ingestion_errors, project) && sbom_report_errors
-
-          pipeline.set_sbom_report_ingestion_errors(sbom_report_errors)
+          track_sbom_report_errors
         end
       end
 
       private
 
+      def strategy_class
+        if sbom_sources.include?(:container_scanning_for_registry)
+          ExecutionStrategy::ContainerScanningForRegistry
+        else
+          ExecutionStrategy::Default
+        end
+      end
+
       attr_reader :pipeline
 
       delegate :project, to: :pipeline, private: true
 
-      # We only ingest one sbom report in container_scanning_for_registry jobs.
-      # This is a temprory quick fix for https://gitlab.com/gitlab-org/gitlab/-/issues/478376.
-      def container_scanning_for_registry_pipeline?
-        valid_sbom_reports&.first&.source&.source_type == :container_scanning_for_registry
-      end
-      strong_memoize_attr :container_scanning_for_registry_pipeline?
+      def sbom_sources
+        sources = Set.new
+
+        valid_sbom_reports.each do |report|
+          sources << report&.source&.source_type
+        end
 
-      def ingest_reports
-        valid_sbom_reports.flat_map { |report| ingest_report(report) }
+        sources.to_a.compact
       end
 
       def all_sbom_reports
         @all_sbom_reports ||= pipeline.sbom_reports(self_and_project_descendants: true).reports
       end
 
+      def track_sbom_report_errors
+        return unless Feature.enabled?(:store_sbom_report_ingestion_errors,
+          project) && sbom_report_errors
+
+        pipeline.set_sbom_report_ingestion_errors(sbom_report_errors)
+      end
+
       def valid_sbom_reports
         all_sbom_reports.select(&:valid?)
       end
+      strong_memoize_attr :valid_sbom_reports
 
       def sbom_report_errors
         return unless invalid_sbom_reports.any?
@@ -74,28 +79,9 @@ def sbom_report_errors
       strong_memoize_attr :sbom_report_errors
 
       def invalid_sbom_reports
-        @invalid_sbom_reports ||= all_sbom_reports.reject(&:valid?)
-      end
-
-      def ingest_report(sbom_report)
-        IngestReportService.execute(pipeline, sbom_report, vulnerabilities_info)
-      end
-
-      def delete_not_present_occurrences(ingested_occurrence_ids)
-        DeleteNotPresentOccurrencesService.execute(pipeline, ingested_occurrence_ids)
-      end
-
-      def vulnerabilities_info
-        return {} if ::Feature.enabled?(:deprecate_vulnerability_occurrence_pipelines, project)
-
-        @vulnerabilities_info ||= Sbom::Ingestion::Vulnerabilities.new(pipeline)
-      end
-
-      def publish_ingested_sbom_event
-        Gitlab::EventStore.publish(
-          Sbom::SbomIngestedEvent.new(data: { pipeline_id: pipeline.id })
-        )
+        all_sbom_reports.reject(&:valid?)
       end
+      strong_memoize_attr :invalid_sbom_reports
 
       def lease_key
         Sbom::Ingestion.project_lease_key(project.id)

ee/spec/services/sbom/ingestion/execution_strategy/container_scanning_for_registry_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Sbom::Ingestion::ExecutionStrategy::ContainerScanningForRegistry, feature_category: :dependency_management do
+  let_it_be(:pipeline) { build_stubbed(:ci_pipeline) }
+  let_it_be(:project) { pipeline.project }
+  let_it_be(:reports) { create_list(:ci_reports_sbom_report, 3) }
+
+  let(:report_ingested_ids) { [[10], [20], [30]] }
+  let(:ingested_ids) { report_ingested_ids.flatten }
+
+  subject(:strategy) { described_class.new(reports, project, pipeline) }
+
+  describe '#execute' do
+    before do
+      reports.zip(report_ingested_ids) do |report, ingested_ids|
+        allow(Sbom::Ingestion::IngestReportService)
+          .to receive(:execute).with(pipeline, report, {})
+          .and_return(ingested_ids)
+      end
+
+      allow(Gitlab::EventStore).to receive(:publish)
+    end
+
+    it 'ingests the reports' do
+      strategy.execute
+
+      reports.each do |report|
+        expect(Sbom::Ingestion::IngestReportService).to have_received(:execute)
+          .with(pipeline, report, {})
+      end
+    end
+
+    it 'publishes the ingested SBOM event with the correct pipeline_id' do
+      strategy.execute
+
+      expect(Gitlab::EventStore).to have_received(:publish).with(
+        an_instance_of(Sbom::SbomIngestedEvent).and(having_attributes(data: hash_including(pipeline_id: pipeline.id)))
+      )
+    end
+  end
+end

ee/spec/services/sbom/ingestion/execution_strategy/default_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Sbom::Ingestion::ExecutionStrategy::Default, feature_category: :dependency_management do
+  let_it_be(:pipeline) { build_stubbed(:ci_pipeline) }
+  let_it_be(:project) { pipeline.project }
+  let_it_be(:reports) { create_list(:ci_reports_sbom_report, 4) }
+
+  let(:report_ingested_ids) { [[1], [2], [3], [4]] }
+  let(:ingested_ids) { report_ingested_ids.flatten }
+
+  subject(:strategy) { described_class.new(reports, project, pipeline) }
+
+  before do
+    allow(project).to receive(:set_latest_ingested_sbom_pipeline_id)
+    allow(Sbom::Ingestion::DeleteNotPresentOccurrencesService).to receive(:execute)
+    allow(Gitlab::EventStore).to receive(:publish)
+  end
+
+  describe '#execute' do
+    before do
+      setup_ingest_report_service
+    end
+
+    it 'ingests the reports with vulnerability info' do
+      strategy.execute
+
+      expect_ingest_report_service_calls
+    end
+
+    it 'sets the latest ingested SBOM pipeline ID' do
+      strategy.execute
+
+      expect(project).to have_received(:set_latest_ingested_sbom_pipeline_id).with(pipeline.id)
+    end
+
+    it 'deletes not present occurrences' do
+      strategy.execute
+
+      expect(Sbom::Ingestion::DeleteNotPresentOccurrencesService).to have_received(:execute).with(pipeline,
+        ingested_ids)
+    end
+
+    context 'when feature flag deprecate_vulnerability_occurrence_pipelines is disabled' do
+      before do
+        stub_feature_flags(deprecate_vulnerability_occurrence_pipelines: false)
+        setup_ingest_report_service(vulnerability: true)
+      end
+
+      it 'ingests the reports with empty vulnerability info' do
+        strategy.execute
+
+        expect_ingest_report_service_calls(vulnerability: true)
+      end
+    end
+
+    context 'when reports are ingested' do
+      it 'publishes the ingested SBOM event' do
+        strategy.execute
+
+        expect(Gitlab::EventStore).to have_received(:publish).with(
+          an_instance_of(Sbom::SbomIngestedEvent).and(having_attributes(data: hash_including(pipeline_id: pipeline.id)))
+        )
+      end
+    end
+
+    context 'when reports are not ingested' do
+      let(:report_ingested_ids) { [[], [], [], []] }
+
+      it 'does not publish the ingested SBOM event' do
+        strategy.execute
+
+        expect(Gitlab::EventStore).not_to have_received(:publish)
+      end
+    end
+
+    context 'when the feature dependency_scanning_using_sbom_reports is disabled' do
+      before do
+        stub_feature_flags(dependency_scanning_using_sbom_reports: false)
+      end
+
+      it 'does not publish the ingested SBOM event' do
+        strategy.execute
+
+        expect(Gitlab::EventStore).not_to have_received(:publish)
+      end
+    end
+  end
+
+  private
+
+  def setup_ingest_report_service(vulnerability: false)
+    reports.zip(report_ingested_ids) do |report, ingested_ids|
+      args = vulnerability ? instance_of(Sbom::Ingestion::Vulnerabilities) : {}
+      allow(Sbom::Ingestion::IngestReportService)
+        .to receive(:execute).with(pipeline, report, args)
+        .and_return(ingested_ids)
+    end
+  end
+
+  def expect_ingest_report_service_calls(vulnerability: false)
+    reports.each do |report|
+      args = vulnerability ? instance_of(Sbom::Ingestion::Vulnerabilities) : {}
+      expect(Sbom::Ingestion::IngestReportService).to have_received(:execute)
+        .with(pipeline, report, args)
+    end
+  end
+end

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

@@ -6,174 +6,76 @@
   let_it_be(:pipeline) { build_stubbed(:ci_pipeline) }
   let_it_be(:reports) { create_list(:ci_reports_sbom_report, 4) }
 
-  let(:sequencer) { ::Ingestion::Sequencer.new }
   let(:wrapper) { instance_double('Gitlab::Ci::Reports::Sbom::Reports') }
-  let(:vulnerability_info) { instance_double('Sbom::Ingestion::Vulnerabilities') }
 
   subject(:execute) { described_class.execute(pipeline) }
 
   before do
     allow(wrapper).to receive(:reports).and_return(reports)
     allow(pipeline).to receive(:sbom_reports).with(self_and_project_descendants: true).and_return(wrapper)
-    allow(::Sbom::Ingestion::Vulnerabilities).to receive(:new).and_return(vulnerability_info)
   end
 
   describe '#execute' do
-    using RSpec::Parameterized::TableSyntax
+    context 'when lease is taken' do
+      include ExclusiveLeaseHelpers
 
-    where(:feature_flag, :feature_flag_stub, :expected_vulnerability_info) do
-      :deprecate_vulnerability_occurrence_pipelines | false | ref(:vulnerability_info)
-      :deprecate_vulnerability_occurrence_pipelines | true | {}
-    end
+      let(:lease_key) { Sbom::Ingestion.project_lease_key(pipeline.project.id) }
 
-    with_them do
       before do
-        stub_feature_flags(feature_flag => feature_flag_stub)
-        allow(::Sbom::Ingestion::DeleteNotPresentOccurrencesService).to receive(:execute)
-        allow(::Sbom::Ingestion::IngestReportService).to receive(:execute)
-                                                           .and_wrap_original do |_, _, report|
-          report.components.map { sequencer.next }
-        end
+        stub_const("#{described_class}::LEASE_TRY_AFTER", 0.01)
+        stub_exclusive_lease_taken(lease_key)
       end
 
-      it 'executes IngestReportService for each report' do
-        reports.each do |report|
-          expect(::Sbom::Ingestion::IngestReportService).to receive(:execute).with(pipeline, report,
-            expected_vulnerability_info)
-        end
-
-        execute
-
-        expect(::Sbom::Ingestion::DeleteNotPresentOccurrencesService).to have_received(:execute)
-                                                                           .with(pipeline, sequencer.range)
+      it 'does not permit parallel execution on the same project' do
+        expect { execute }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError)
       end
+    end
 
-      context 'when lease is taken' do
-        include ExclusiveLeaseHelpers
-
-        let(:lease_key) { Sbom::Ingestion.project_lease_key(pipeline.project.id) }
-
-        before do
-          # Speed up retries to avoid long-running tests
-          stub_const("#{described_class}::LEASE_TRY_AFTER", 0.01)
-          stub_exclusive_lease_taken(lease_key)
-        end
+    context 'when a report is invalid' do
+      let_it_be(:invalid_report) { create(:ci_reports_sbom_report, :invalid) }
+      let_it_be(:reports) { [invalid_report] + create_list(:ci_reports_sbom_report, 4) }
 
-        it 'does not permit parallel execution on the same project' do
-          expect { execute }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError)
+      it 'does not process the invalid report' do
+        execute
 
-          expect(::Sbom::Ingestion::IngestReportService).not_to have_received(:execute)
-        end
+        expect(pipeline.sbom_report_ingestion_errors).to include(invalid_report.errors)
       end
+    end
 
-      context 'when feature flag store_sbom_report_ingestion_errors is enabled' do
-        before do
-          stub_feature_flags(store_sbom_report_ingestion_errors: true)
-        end
-
-        it 'does not set SBOM ingestion errors when report is valid' do
-          allow(pipeline).to receive(:set_sbom_report_ingestion_errors)
-
-          execute
-
-          expect(pipeline).not_to have_received(:set_sbom_report_ingestion_errors)
-        end
+    context 'when feature flag store_sbom_report_ingestion_errors is disabled' do
+      before do
+        stub_feature_flags(store_sbom_report_ingestion_errors: false)
       end
 
-      context 'when feature flag dependency_scanning_using_sbom_reports is enabled' do
-        it 'publishes the pipeline id to the event store' do
-          expect { execute }.to publish_event(::Sbom::SbomIngestedEvent).with({ pipeline_id: pipeline.id })
-        end
-      end
+      it 'does not set the ingestion errors' do
+        allow(pipeline).to receive(:set_sbom_report_ingestion_errors)
 
-      context 'when feature flag dependency_scanning_using_sbom_reports is disabled' do
-        before do
-          stub_feature_flags(dependency_scanning_using_sbom_reports: false)
-        end
+        execute
 
-        it 'does not publish anything to the event store' do
-          expect(Gitlab::EventStore).not_to receive(:publish)
-        end
+        expect(pipeline).not_to have_received(:set_sbom_report_ingestion_errors)
       end
+    end
 
-      context 'when a report is invalid' do
-        let_it_be(:sbom_ingestion_error) { 'Unsupported CycloneDX version' }
-        let_it_be(:invalid_report) { create(:ci_reports_sbom_report, :invalid, error: sbom_ingestion_error) }
-        let_it_be(:valid_reports) { create_list(:ci_reports_sbom_report, 4) }
-        let_it_be(:reports) { [invalid_report] + valid_reports }
-
-        it 'does not process the invalid report' do
-          expect(::Sbom::Ingestion::IngestReportService).not_to receive(:execute).with(pipeline,
-            invalid_report,
-            vulnerability_info)
-
-          valid_reports.each do |report|
-            expect(::Sbom::Ingestion::IngestReportService).to receive(:execute).with(pipeline, report,
-              expected_vulnerability_info)
-          end
-
-          execute
-
-          expect(::Sbom::Ingestion::DeleteNotPresentOccurrencesService).to have_received(:execute)
-                                                                             .with(pipeline, sequencer.range)
-        end
-
-        context 'when feature flag store_sbom_report_ingestion_errors is enabled' do
-          before do
-            stub_feature_flags(store_sbom_report_ingestion_errors: true)
-          end
-
-          it 'sets the ingestion errors' do
-            execute
+    context 'when a report source is container_scanning_for_registry' do
+      let_it_be(:registry_sources) { create(:ci_reports_sbom_source, :container_scanning_for_registry) }
+      let_it_be(:reports) { [create(:ci_reports_sbom_report, source: registry_sources)] }
 
-            expect(pipeline.has_sbom_report_ingestion_errors?).to be_truthy
-            expect(pipeline.sbom_report_ingestion_errors).to eq([[sbom_ingestion_error]])
-          end
+      it 'uses ContainerScanningForRegistry strategy' do
+        expect_next_instance_of(Sbom::Ingestion::ExecutionStrategy::ContainerScanningForRegistry) do |instance|
+          expect(instance).to receive(:execute)
         end
 
-        context 'when feature flag store_sbom_report_ingestion_errors is disabled' do
-          before do
-            stub_feature_flags(store_sbom_report_ingestion_errors: false)
-            allow(pipeline).to receive(:set_sbom_report_ingestion_errors)
-          end
-
-          it 'does not set the ingestion errors' do
-            execute
-
-            expect(pipeline).not_to have_received(:set_sbom_report_ingestion_errors)
-          end
-        end
-      end
-
-      context 'when a report source is container_scanning_for_registry' do
-        let_it_be(:sbom_ingestion_error) { 'Unsupported CycloneDX version' }
-        let_it_be(:registry_sources) { create(:ci_reports_sbom_source, :container_scanning_for_registry) }
-        let_it_be(:reports) { [create(:ci_reports_sbom_report, source: registry_sources)] }
-
-        it 'ingest the report but does not delete the existing occurrences' do
-          expect(::Sbom::Ingestion::IngestReportService).to receive(:execute).with(pipeline,
-            reports.first,
-            expected_vulnerability_info)
-
-          execute
-
-          expect(::Sbom::Ingestion::DeleteNotPresentOccurrencesService).not_to have_received(:execute)
-                                                                                 .with(pipeline, sequencer.range)
-        end
+        execute
       end
+    end
 
-      describe 'setting the latest ingested SBOM pipeline ID' do
-        let(:project) { pipeline.project }
-
-        before do
-          allow(project).to receive(:set_latest_ingested_sbom_pipeline_id)
+    context 'when there are no container_scanning_for_registry sources' do
+      it 'uses Default strategy' do
+        expect_next_instance_of(Sbom::Ingestion::ExecutionStrategy::Default) do |instance|
+          expect(instance).to receive(:execute)
         end
 
-        it 'sets the latest ingested SBOM pipeline ID' do
-          execute
-
-          expect(project).to have_received(:set_latest_ingested_sbom_pipeline_id).with(pipeline.id)
-        end
+        execute
       end
     end
   end