Implement suggestions based on reviewer feedback

ee/app/finders/security/jobs_finder.rb

@@ -15,39 +15,39 @@
 
 module Security
   class JobsFinder
+    include Gitlab::Utils::StrongMemoize
+
     attr_reader :pipeline
 
+    JOB_TYPES = [:sast, :dast, :dependency_scanning, :container_scanning].freeze
+
     def initialize(pipeline, params = { all: true })
       @pipeline = pipeline
       @params = params
     end
 
     def execute
-      job_types = all_secure_jobs
-      job_types = by_specific_job_type(job_types, :sast)
-      job_types = by_specific_job_type(job_types, :dast)
-      job_types = by_specific_job_type(job_types, :dependency_scanning)
-      job_types = by_specific_job_type(job_types, :container_scanning)
-
-      return [] if job_types.empty?
-
-      Feature.enabled?(:ci_build_metadata_config) ? find_jobs(job_types) : find_jobs_legacy(job_types)
-    end
+      return [] if job_types_for_processing.empty?
 
-    def all_secure_jobs
-      if @params[:all]
-        return [:sast, :dast, :dependency_scanning, :container_scanning]
+      if Feature.enabled?(:ci_build_metadata_config)
+        find_jobs(job_types_for_processing)
+      else
+        find_jobs_legacy(job_types_for_processing)
       end
-
-      []
     end
 
-    def by_specific_job_type(job_types, job_type)
-      if @params[job_type]
-        return job_types + [job_type]
-      end
+    private
 
-      job_types
+    def job_types_for_processing
+      strong_memoize(:job_types_for_processing) do
+        if @params[:all]
+          JOB_TYPES
+        else
+          JOB_TYPES.each_with_object([]) do |job_type, job_types|
+            job_types << job_type if @params[job_type]
+          end
+        end
+      end
     end
 
     def find_jobs(job_types)
@@ -55,12 +55,15 @@ module Security
     end
 
     def find_jobs_legacy(job_types)
+      # first job type is added as a WHERE statement
       query = @pipeline.builds.with_secure_report_legacy(job_types.first)
 
+      # following job types are added as OR statements
       jobs = job_types.drop(1).reduce(query) do |qry, job_type|
         qry.or(@pipeline.builds.with_secure_report_legacy(job_type))
       end
 
+      # the query doesn't guarantee accuracy, so we verify it here
       jobs.select do |job|
         job_types.find { |job_type| job.options.dig(:artifacts, :reports, job_type) }
       end

ee/spec/finders/security/jobs_finder_spec.rb

@@ -19,25 +19,33 @@ describe Security::JobsFinder do
       end
 
       context 'with non secure jobs' do
-        let!(:build) { create(:ci_build, pipeline: pipeline) }
+        before do
+          create(:ci_build, pipeline: pipeline)
+        end
 
         it { is_expected.to be_empty }
       end
 
       context 'with jobs having report artifacts' do
-        let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { file: 'test.file' } }) }
+        before do
+          create(:ci_build, pipeline: pipeline, options: { artifacts: { file: 'test.file' } })
+        end
 
         it { is_expected.to be_empty }
       end
 
       context 'with jobs having non secure report artifacts' do
-        let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'test.file' } } }) }
+        before do
+          create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'test.file' } } })
+        end
 
         it { is_expected.to be_empty }
       end
 
-      context 'with jobs having almost secure like report artifacts' do
-        let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'sast.file' } } }) }
+      context 'with jobs having report artifacts that are similar to secure artifacts' do
+        before do
+          create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'sast.file' } } })
+        end
 
         it { is_expected.to be_empty }
       end
@@ -67,8 +75,9 @@ describe Security::JobsFinder do
       end
 
       context 'with many secure pipelines' do
-        let!(:another_pipeline) { create(:ci_pipeline) }
-        let!(:another_build) { create(:ci_build, :dast, pipeline: another_pipeline) }
+        before do
+          create(:ci_build, :dast, pipeline: create(:ci_pipeline))
+        end
 
         let!(:build) { create(:ci_build, :dast, pipeline: pipeline) }
 
@@ -78,16 +87,16 @@ describe Security::JobsFinder do
       end
 
       context 'with specific secure job types' do
-        let!(:build_a) { create(:ci_build, :sast, pipeline: pipeline) }
-        let!(:build_b) { create(:ci_build, :container_scanning, pipeline: pipeline) }
-        let!(:build_c) { create(:ci_build, :dast, pipeline: pipeline) }
+        let!(:sast_build) { create(:ci_build, :sast, pipeline: pipeline) }
+        let!(:container_scanning_build) { create(:ci_build, :container_scanning, pipeline: pipeline) }
+        let!(:dast_build) { create(:ci_build, :dast, pipeline: pipeline) }
 
         let(:finder) { described_class.new(pipeline, { sast: true, container_scanning: true }) }
 
         it 'returns only those requested' do
-          is_expected.to include(build_a)
-          is_expected.to include(build_b)
-          is_expected.not_to include(build_c)
+          is_expected.to include(sast_build)
+          is_expected.to include(container_scanning_build)
+          is_expected.not_to include(dast_build)
         end
       end
     end
@@ -102,19 +111,25 @@ describe Security::JobsFinder do
       end
 
       context 'with non secure jobs' do
-        let!(:build) { create(:ci_build, pipeline: pipeline) }
+        before do
+          create(:ci_build, pipeline: pipeline)
+        end
 
         it { is_expected.to be_empty }
       end
 
       context 'with jobs having report artifacts' do
-        let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { file: 'test.file' } }) }
+        before do
+          create(:ci_build, pipeline: pipeline, options: { artifacts: { file: 'test.file' } })
+        end
 
         it { is_expected.to be_empty }
       end
 
       context 'with jobs having non secure report artifacts' do
-        let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'test.file' } } }) }
+        before do
+          create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'test.file' } } })
+        end
 
         it { is_expected.to be_empty }
       end
@@ -144,8 +159,9 @@ describe Security::JobsFinder do
       end
 
       context 'with many secure pipelines' do
-        let!(:another_pipeline) { create(:ci_pipeline) }
-        let!(:another_build) { create(:ci_build, :dast, pipeline: another_pipeline) }
+        before do
+          create(:ci_build, :dast, pipeline: create(:ci_pipeline))
+        end
 
         let!(:build) { create(:ci_build, :dast, pipeline: pipeline) }
 
@@ -155,16 +171,16 @@ describe Security::JobsFinder do
       end
 
       context 'with specific secure job types' do
-        let!(:build_a) { create(:ci_build, :sast, pipeline: pipeline) }
-        let!(:build_b) { create(:ci_build, :container_scanning, pipeline: pipeline) }
-        let!(:build_c) { create(:ci_build, :dast, pipeline: pipeline) }
+        let!(:sast_build) { create(:ci_build, :sast, pipeline: pipeline) }
+        let!(:container_scanning_build) { create(:ci_build, :container_scanning, pipeline: pipeline) }
+        let!(:dast_build) { create(:ci_build, :dast, pipeline: pipeline) }
 
         let(:finder) { described_class.new(pipeline, { sast: true, container_scanning: true }) }
 
         it 'returns only those requested' do
-          is_expected.to include(build_a)
-          is_expected.to include(build_b)
-          is_expected.not_to include(build_c)
+          is_expected.to include(sast_build)
+          is_expected.to include(container_scanning_build)
+          is_expected.not_to include(dast_build)
         end
       end
     end