Merge branch 'improve_vuln_tracking-handle_uniq_errors' into 'master'

Update StoreReportService to better handle RecordNotUnique errors when using signatures

See merge request gitlab-org/gitlab!61269

ee/app/services/security/store_report_service.rb

@@ -139,11 +139,11 @@ module Security
         # This might happen if we're processing another report in parallel and it finds the same Finding
         # faster. In that case we need to perform the lookup again
 
-        by_uuid = project.vulnerability_findings.reset.find_by(uuid: finding.uuid)
-        return by_uuid if by_uuid
+        vulnerability_finding = project.vulnerability_findings.reset.find_by(uuid: finding.uuid)
+        return vulnerability_finding if vulnerability_finding
 
-        by_find_params = project.vulnerability_findings.reset.find_by(find_params)
-        return by_find_params if by_find_params
+        vulnerability_finding = project.vulnerability_findings.reset.find_by(find_params)
+        return vulnerability_finding if vulnerability_finding
 
         Gitlab::ErrorTracking.track_and_raise_exception(e, find_params: find_params, uuid: finding.uuid)
       rescue ActiveRecord::ActiveRecordError => e
@@ -175,32 +175,46 @@ module Security
       begin
         vulnerability_finding = matched_findings.first
         if vulnerability_finding.nil?
-          find_params[:uuid] = finding.uuid
           vulnerability_finding = project
             .vulnerability_findings
-            .create_with(create_params)
-            .find_or_initialize_by(find_params)
+            .create_with(create_params.merge(find_params))
+            .new
         end
 
-        vulnerability_finding.uuid = finding.uuid
-
-        vulnerability_finding.location_fingerprint = if finding.signatures.empty?
-                                                       finding.location.fingerprint
-                                                     else
-                                                       finding.signatures.max_by(&:priority).signature_hex
-                                                     end
-
-        vulnerability_finding.location = create_params.dig(:location)
+        sync_vulnerability_finding(vulnerability_finding, finding, create_params.dig(:location))
         vulnerability_finding.save!
 
         vulnerability_finding
-      rescue ActiveRecord::RecordNotUnique
-        get_matched_findings(finding, normalized_signatures, find_params).first
+      rescue ActiveRecord::RecordNotUnique => e
+        # the uuid is the only unique constraint on the vulnerability_occurrences
+        # table - no need to use get_matched_findings(...).first here. Fetching
+        # the finding with the same uuid will be enough
+        vulnerability_finding = project.vulnerability_findings.reset.find_by(uuid: finding.uuid)
+        if vulnerability_finding
+          sync_vulnerability_finding(vulnerability_finding, finding, create_params.dig(:location))
+          vulnerability_finding.save!
+          return vulnerability_finding
+        end
+
+        Gitlab::ErrorTracking.track_and_raise_exception(e, find_params: find_params, uuid: finding.uuid)
       rescue ActiveRecord::RecordInvalid => e
         Gitlab::ErrorTracking.track_and_raise_exception(e, create_params: create_params&.dig(:raw_metadata))
       end
     end
 
+    def sync_vulnerability_finding(vulnerability_finding, finding, location_data)
+      vulnerability_finding.uuid = finding.uuid
+      vulnerability_finding.location_fingerprint = fingerprint_for(finding)
+
+      vulnerability_finding.location = location_data
+    end
+
+    def fingerprint_for(finding)
+      return finding.location.fingerprint if finding.signatures.empty?
+
+      finding.signatures.max_by(&:priority).signature_hex
+    end
+
     def update_vulnerability_scanner(finding)
       scanner = scanners_objects[finding.scanner.key]
       scanner.update!(finding.scanner.to_hash)

ee/spec/services/security/store_report_service_spec.rb

@@ -376,6 +376,45 @@ RSpec.describe Security::StoreReportService, '#execute' do
         expect(finding.reload).to have_attributes(severity: 'medium', name: 'Cipher with no integrity')
       end
 
+      context 'when RecordNotUnique error has been raised' do
+        let(:report_finding) { report.findings.find { |f| f.location.fingerprint == finding.location_fingerprint} }
+
+        subject(:store_report_service) { described_class.new(new_pipeline, new_report) }
+
+        before do
+          allow(store_report_service).to receive(:get_matched_findings).and_wrap_original do |orig_method, other_finding, *args|
+            if other_finding.uuid != report_finding.uuid
+              orig_method.call(other_finding, *args)
+            else
+              finding.update!(name: 'SHOULD BE UPDATED', uuid: report_finding.uuid)
+              []
+            end
+          end
+
+          allow(Gitlab::ErrorTracking).to receive(:track_and_raise_exception).and_call_original
+        end
+
+        it 'handles the error correctly' do
+          next unless vulnerability_finding_signatures_enabled
+
+          report_finding = report.findings.find { |f| f.location.fingerprint == finding.location_fingerprint}
+
+          store_report_service.execute
+
+          expect(finding.reload.name).to eq(report_finding.name)
+        end
+
+        it 'raises the error if there exists no vulnerability finding' do
+          next unless vulnerability_finding_signatures_enabled
+
+          allow(store_report_service).to receive(:sync_vulnerability_finding).and_raise(ActiveRecord::RecordNotUnique)
+
+          expect { store_report_service.execute }.to raise_error { |error|
+            expect(Gitlab::ErrorTracking).to have_received(:track_and_raise_exception).with(error, any_args)
+          }
+        end
+      end
+
       it 'updates signatures to match new values' do
         next unless vulnerability_finding_signatures_enabled