Merge branch 'fix_bug_n+1' into 'master'

Fix ArgumentError for bulk insert

See merge request gitlab-org/gitlab!60543

ee/app/services/security/store_report_service.rb

@@ -257,8 +257,11 @@ module Security
     def update_vulnerabilities_identifiers
       vulnerability_finding_to_finding_map.keys.in_groups_of(BATCH_SIZE, false) do |vulnerability_findings|
         identifier_object_records = get_vulnerability_identifier_objects_for(vulnerability_findings)
-        insert_new_vulnerability_identifiers_for(identifier_object_records)
-        update_existing_vulnerability_identifiers_for(identifier_object_records)
+        records_with_id, records_without_id = identifier_object_records
+                                                .partition { |identifier| identifier[:id].present? }
+
+        update_existing_vulnerability_identifiers_for(records_with_id)
+        insert_new_vulnerability_identifiers_for(records_without_id)
       end
     rescue StandardError => e
       Gitlab::ErrorTracking.track_exception(e)
@@ -281,13 +284,15 @@ module Security
     end
 
     def insert_new_vulnerability_identifiers_for(identifier_object_records)
-      identifier_object_records_without_id = identifier_object_records.select { |identifier| identifier[:id].nil? }.uniq
-      Vulnerabilities::Identifier.insert_all(identifier_object_records_without_id) if identifier_object_records_without_id.present?
+      identifier_object_records = identifier_object_records.uniq.group_by(&:keys).values
+
+      identifier_object_records.each { |records| Vulnerabilities::Identifier.insert_all(records) }
     end
 
     def update_existing_vulnerability_identifiers_for(identifier_object_records)
-      identifier_object_records_with_id = identifier_object_records.select { |identifier| identifier[:id].present? }.uniq
-      Vulnerabilities::Identifier.upsert_all(identifier_object_records_with_id) if identifier_object_records_with_id.present?
+      identifier_object_records = identifier_object_records.uniq.group_by(&:keys).values
+
+      identifier_object_records.each { |records| Vulnerabilities::Identifier.upsert_all(records) }
     end
 
     def update_vulnerabilities_finding_identifiers

ee/changelogs/unreleased/fix_bug_n-1.yml

+---
+title: Fix ArgumentError for bulk insert
+merge_request: 60543
+author:
+type: fixed

ee/spec/services/security/store_report_service_spec.rb

@@ -106,6 +106,45 @@ RSpec.describe Security::StoreReportService, '#execute' do
         end
       end
 
+      context 'when some attributes are missing in the identifiers' do
+        let(:trait) { :sast }
+        let(:other_params) {{ external_type: 'find_sec_bugs_type', external_id: 'PREDICTABLE_RANDOM', name: 'Find Security Bugs-PREDICTABLE_RANDOM', url: 'https://find-sec-bugs.github.io/bugs.htm#PREDICTABLE_RANDOM', created_at: Time.current, updated_at: Time.current }}
+        let(:record_1) {{ id: 4, project_id: 2, fingerprint: '5848739446034d982ef7beece3bb19bff4044ffb', **other_params }}
+        let(:record_2) {{ project_id: 2, fingerprint: '5848739446034d982ef7beece3bb19bff4044ffb', **other_params }}
+        let(:record_3) {{ id: 4, fingerprint: '5848739446034d982ef7beece3bb19bff4044ffb', **other_params }}
+        let(:record_4) {{ id: 5, fingerprint: '6848739446034d982ef7beece3bb19bff4044ffb', **other_params }}
+        let(:record_5) {{ fingerprint: '5848739446034d982ef7beece3bb19bff4044ffb', **other_params }}
+        let(:record_6) {{ fingerprint: '6848739446034d982ef7beece3bb19bff4044ffb', **other_params }}
+
+        subject { described_class.new(pipeline, report) }
+
+        it 'updates existing vulnerability identifiers in groups' do
+          expect(Vulnerabilities::Identifier).to receive(:upsert_all).with([record_1])
+          expect(Vulnerabilities::Identifier).to receive(:upsert_all).with([record_3, record_4])
+
+          subject.send(:update_existing_vulnerability_identifiers_for, [record_1, record_3, record_4])
+        end
+
+        it 'does not update any identifier for an empty list of records' do
+          expect(Vulnerabilities::Identifier).not_to receive(:upsert_all)
+
+          subject.send(:update_existing_vulnerability_identifiers_for, [])
+        end
+
+        it 'inserts new vulnerability identifiers in groups' do
+          expect(Vulnerabilities::Identifier).to receive(:insert_all).with([record_2])
+          expect(Vulnerabilities::Identifier).to receive(:insert_all).with([record_5, record_6])
+
+          subject.send(:insert_new_vulnerability_identifiers_for, [record_2, record_5, record_6])
+        end
+
+        it 'does not insert any identifier for an empty list of records' do
+          expect(Vulnerabilities::Identifier).not_to receive(:insert_all)
+
+          subject.send(:insert_new_vulnerability_identifiers_for, [])
+        end
+      end
+
       context 'when N+1 database queries have been removed' do
         let(:trait) { :sast }
         let(:bandit_scanner) { build(:ci_reports_security_scanner, external_id: 'bandit', name: 'Bandit') }