vulnerability_id processing: separate import/reimport

Author: valentijnscholten

dojo/importers/base_importer.py

@@ -786,51 +786,23 @@ def process_cve(
 
         return finding
 
-    def process_vulnerability_ids(
+    def store_vulnerability_ids(
         self,
         finding: Finding,
     ) -> Finding:
         """
-        Parse the `unsaved_vulnerability_ids` field from findings after they are parsed
-        to create `Vulnerability_Id` objects with the finding associated correctly.
-        Only updates if vulnerability_ids have changed to avoid unnecessary database operations.
-
-        Note: The finding parameter can be:
-        - A new finding (from import) with unsaved_vulnerability_ids set from the parsed report
-        - An existing finding (from reimport) with unsaved_vulnerability_ids copied from the
-          finding_from_report before calling this method
+        Store vulnerability IDs for a finding.
+        Reads from finding.unsaved_vulnerability_ids and saves them overwriting existing ones.
 
         Args:
-            finding: The finding to process vulnerability IDs for.
-                For reimports, this is the existing finding with unsaved_vulnerability_ids
-                set from the import/reimport report.
+            finding: The finding to store vulnerability IDs for
 
         Returns:
             The finding object
 
         """
-        # Normalize to empty list if None - always process vulnerability IDs
         vulnerability_ids_to_process = finding.unsaved_vulnerability_ids or []
-
-        # For existing findings, check if there are changes before updating
-        if finding.pk and len(finding.vulnerability_id_set.all()) > 0:
-            # Get existing vulnerability IDs from the database
-            existing_vuln_ids = set(finding.vulnerability_ids) if finding.vulnerability_ids else set()
-            # Normalize the new vulnerability IDs (remove duplicates for comparison)
-            new_vuln_ids = set(vulnerability_ids_to_process)
-
-            # Only update if vulnerability IDs have changed
-            if existing_vuln_ids == new_vuln_ids:
-                logger.debug(
-                    f"Skipping vulnerability_ids update for finding {finding.id} - "
-                    f"vulnerability_ids unchanged: {sorted(existing_vuln_ids)}",
-                )
-                return finding
-
-        # Use the helper function which handles deletion and creation
-        # Always use delete_existing=True - it's a no-op if there are no existing IDs
-        finding_helper.save_vulnerability_ids(finding, vulnerability_ids_to_process, delete_existing=True)
-
+        finding_helper.save_vulnerability_ids(finding, vulnerability_ids_to_process, delete_existing=False)
         return finding
 
     def process_files(

dojo/importers/default_importer.py

@@ -234,7 +234,7 @@ def process_findings(
             # Process any files
             self.process_files(finding)
             # Process vulnerability IDs
-            finding = self.process_vulnerability_ids(finding)
+            finding = self.store_vulnerability_ids(finding)
             # Categorize this finding as a new one
             new_findings.append(finding)
             # all data is already saved on the finding, we only need to trigger post processing in batches

dojo/importers/default_reimporter.py

@@ -175,6 +175,9 @@ def process_findings(
         else:
             original_findings = self.test.finding_set.all().filter(Q(service__isnull=True) | Q(service__exact=""))
 
+        # Prefetch vulnerability_id_set for reconcile_vulnerability_ids
+        original_findings = original_findings.prefetch_related("vulnerability_id_set")
+
         logger.debug(f"original_findings_qyer: {original_findings.query}")
         self.original_items = list(original_findings)
         logger.debug(f"original_items: {[(item.id, item.hash_code) for item in self.original_items]}")
@@ -691,6 +694,41 @@ def process_finding_that_was_not_matched(
         self.process_request_response_pairs(unsaved_finding)
         return unsaved_finding
 
+    def reconcile_vulnerability_ids(
+        self,
+        finding: Finding,
+    ) -> Finding:
+        """
+        Reconcile vulnerability IDs for an existing finding.
+        Checks if IDs have changed before updating to avoid unnecessary database operations.
+        Uses prefetched data if available, otherwise fetches efficiently.
+
+        Args:
+            finding: The existing finding to reconcile vulnerability IDs for.
+                Must have unsaved_vulnerability_ids set.
+
+        Returns:
+            The finding object
+
+        """
+        vulnerability_ids_to_process = finding.unsaved_vulnerability_ids or []
+
+        # Use prefetched data directly without triggering queries
+        existing_vuln_ids = {v.vulnerability_id for v in finding.vulnerability_id_set.all()}
+        new_vuln_ids = set(vulnerability_ids_to_process)
+
+        # Early exit if unchanged
+        if existing_vuln_ids == new_vuln_ids:
+            logger.debug(
+                f"Skipping vulnerability_ids update for finding {finding.id} - "
+                f"vulnerability_ids unchanged: {sorted(existing_vuln_ids)}",
+            )
+            return finding
+
+        # Update if changed
+        finding_helper.save_vulnerability_ids(finding, vulnerability_ids_to_process, delete_existing=True)
+        return finding
+
     def finding_post_processing(
         self,
         finding: Finding,
@@ -716,13 +754,13 @@ def finding_post_processing(
         self.process_files(finding)
         # Process vulnerability IDs
         # Copy unsaved_vulnerability_ids from the report finding to the existing finding
-        # so process_vulnerability_ids can process them (see its docstring for details)
+        # so reconcile_vulnerability_ids can process them
         # Always set it (even if empty list) so we can clear existing IDs when report has none
         finding.unsaved_vulnerability_ids = finding_from_report.unsaved_vulnerability_ids or []
         # Store the current cve value to check if it changes
         old_cve = finding.cve
         # legacy cve field has already been processed/set earlier
-        finding = self.process_vulnerability_ids(finding)
+        finding = self.reconcile_vulnerability_ids(finding)
         # Save the finding only if the cve field was changed by save_vulnerability_ids
         # This is temporary as the cve field will be phased out
         if finding.cve != old_cve:

unittests/test_importers_importer.py

@@ -7,6 +7,7 @@
 from rest_framework.test import APIClient
 
 from dojo.importers.default_importer import DefaultImporter
+from dojo.importers.default_reimporter import DefaultReImporter
 from dojo.models import (
     Development_Environment,
     Engagement,
@@ -562,16 +563,15 @@ def create_default_data(self):
             "scan_type": NPM_AUDIT_SCAN_TYPE,
         }
 
-    @patch("dojo.importers.base_importer.Vulnerability_Id", autospec=True)
-    def test_handle_vulnerability_ids_references_and_cve(self, mock):
+    def test_handle_vulnerability_ids_references_and_cve(self):
         # Why doesn't this test use the test db and query for one?
         vulnerability_ids = ["CVE", "REF-1", "REF-2"]
         finding = Finding()
         finding.unsaved_vulnerability_ids = vulnerability_ids
         finding.test = self.test
         finding.reporter = self.testuser
         finding.save()
-        DefaultImporter(**self.importer_data).process_vulnerability_ids(finding)
+        DefaultImporter(**self.importer_data).store_vulnerability_ids(finding)
 
         self.assertEqual("CVE", finding.vulnerability_ids[0])
         self.assertEqual("CVE", finding.cve)
@@ -580,45 +580,42 @@ def test_handle_vulnerability_ids_references_and_cve(self, mock):
         self.assertEqual("REF-2", finding.vulnerability_ids[2])
         finding.delete()
 
-    @patch("dojo.importers.base_importer.Vulnerability_Id", autospec=True)
-    def test_handle_no_vulnerability_ids_references_and_cve(self, mock):
+    def test_handle_no_vulnerability_ids_references_and_cve(self):
         vulnerability_ids = ["CVE"]
         finding = Finding()
         finding.test = self.test
         finding.reporter = self.testuser
         finding.save()
         finding.unsaved_vulnerability_ids = vulnerability_ids
 
-        DefaultImporter(**self.importer_data).process_vulnerability_ids(finding)
+        DefaultImporter(**self.importer_data).store_vulnerability_ids(finding)
 
         self.assertEqual("CVE", finding.vulnerability_ids[0])
         self.assertEqual("CVE", finding.cve)
         self.assertEqual(vulnerability_ids, finding.unsaved_vulnerability_ids)
         finding.delete()
 
-    @patch("dojo.importers.base_importer.Vulnerability_Id", autospec=True)
-    def test_handle_vulnerability_ids_references_and_no_cve(self, mock):
+    def test_handle_vulnerability_ids_references_and_no_cve(self):
         vulnerability_ids = ["REF-1", "REF-2"]
         finding = Finding()
         finding.test = self.test
         finding.reporter = self.testuser
         finding.save()
         finding.unsaved_vulnerability_ids = vulnerability_ids
-        DefaultImporter(**self.importer_data).process_vulnerability_ids(finding)
+        DefaultImporter(**self.importer_data).store_vulnerability_ids(finding)
 
         self.assertEqual("REF-1", finding.vulnerability_ids[0])
         self.assertEqual("REF-1", finding.cve)
         self.assertEqual(vulnerability_ids, finding.unsaved_vulnerability_ids)
         self.assertEqual("REF-2", finding.vulnerability_ids[1])
         finding.delete()
 
-    @patch("dojo.importers.base_importer.Vulnerability_Id", autospec=True)
-    def test_no_handle_vulnerability_ids_references_and_no_cve(self, mock):
+    def test_no_handle_vulnerability_ids_references_and_no_cve(self):
         finding = Finding()
         finding.test = self.test
         finding.reporter = self.testuser
         finding.save()
-        DefaultImporter(**self.importer_data).process_vulnerability_ids(finding)
+        DefaultImporter(**self.importer_data).store_vulnerability_ids(finding)
         self.assertEqual(finding.cve, None)
         self.assertEqual(finding.unsaved_vulnerability_ids, None)
         self.assertEqual(finding.vulnerability_ids, [])
@@ -644,7 +641,7 @@ def test_clear_vulnerability_ids_on_empty_list(self):
 
         # Process with empty list - should clear all IDs
         finding.unsaved_vulnerability_ids = []
-        DefaultImporter(**self.importer_data).process_vulnerability_ids(finding)
+        DefaultReImporter(test=self.test, environment=self.importer_data["environment"], scan_type=self.importer_data["scan_type"]).reconcile_vulnerability_ids(finding)
         # Save the finding to persist the cve=None change
         finding.save()
 
@@ -681,7 +678,7 @@ def test_change_vulnerability_ids_on_reimport(self):
         # Process with different IDs - should replace old IDs
         new_vulnerability_ids = ["CVE-2021-9999", "GHSA-xxxx-yyyy"]
         finding.unsaved_vulnerability_ids = new_vulnerability_ids
-        DefaultImporter(**self.importer_data).process_vulnerability_ids(finding)
+        DefaultReImporter(test=self.test, environment=self.importer_data["environment"], scan_type=self.importer_data["scan_type"]).reconcile_vulnerability_ids(finding)
         # Save the finding to persist the cve change
         finding.save()
 

unittests/test_importers_performance.py

@@ -177,9 +177,9 @@ def test_import_reimport_reimport_performance_async(self):
         self._import_reimport_performance(
             expected_num_queries1=340,
             expected_num_async_tasks1=7,
-            expected_num_queries2=288,
+            expected_num_queries2=294,
             expected_num_async_tasks2=18,
-            expected_num_queries3=175,
+            expected_num_queries3=180,
             expected_num_async_tasks3=17,
         )
 
@@ -195,9 +195,9 @@ def test_import_reimport_reimport_performance_pghistory_async(self):
         self._import_reimport_performance(
             expected_num_queries1=306,
             expected_num_async_tasks1=7,
-            expected_num_queries2=281,
+            expected_num_queries2=287,
             expected_num_async_tasks2=18,
-            expected_num_queries3=170,
+            expected_num_queries3=175,
             expected_num_async_tasks3=17,
         )
 
@@ -219,9 +219,9 @@ def test_import_reimport_reimport_performance_no_async(self):
         self._import_reimport_performance(
             expected_num_queries1=346,
             expected_num_async_tasks1=6,
-            expected_num_queries2=294,
+            expected_num_queries2=300,
             expected_num_async_tasks2=17,
-            expected_num_queries3=181,
+            expected_num_queries3=186,
             expected_num_async_tasks3=16,
         )
 
@@ -241,9 +241,9 @@ def test_import_reimport_reimport_performance_pghistory_no_async(self):
         self._import_reimport_performance(
             expected_num_queries1=312,
             expected_num_async_tasks1=6,
-            expected_num_queries2=287,
+            expected_num_queries2=293,
             expected_num_async_tasks2=17,
-            expected_num_queries3=176,
+            expected_num_queries3=181,
             expected_num_async_tasks3=16,
         )
 
@@ -267,9 +267,9 @@ def test_import_reimport_reimport_performance_no_async_with_product_grading(self
         self._import_reimport_performance(
             expected_num_queries1=348,
             expected_num_async_tasks1=8,
-            expected_num_queries2=296,
+            expected_num_queries2=302,
             expected_num_async_tasks2=19,
-            expected_num_queries3=183,
+            expected_num_queries3=188,
             expected_num_async_tasks3=18,
         )
 
@@ -290,9 +290,9 @@ def test_import_reimport_reimport_performance_pghistory_no_async_with_product_gr
         self._import_reimport_performance(
             expected_num_queries1=314,
             expected_num_async_tasks1=8,
-            expected_num_queries2=289,
+            expected_num_queries2=295,
             expected_num_async_tasks2=19,
-            expected_num_queries3=178,
+            expected_num_queries3=183,
             expected_num_async_tasks3=18,
         )