448 none package uid (#454)

* Scope the packages QuerySet by current project #448 Signed-off-by: Thomas Druez <[email protected]> * Bump scancode to 31.0.0rc2 #448 * The new scancode version provides a fix to the error encountered in #448 * Update expected test results Signed-off-by: Jono Yang <[email protected]> * Ensure package_uid unique and indexed #448 * TODO: Do we want to make blank and null true on the field since we have existing empty values and the pipelines do not set a package_uid when creating a DiscoveredPackage? Signed-off-by: Jono Yang <[email protected]> * Make DiscoveredPackage.package_uid unique within a Project #448 Signed-off-by: Thomas Druez <[email protected]> * Simplify update_or_create_package and fix unit test #448 Add a override option to update_from_data Signed-off-by: Thomas Druez <[email protected]> Co-authored-by: Thomas Druez <[email protected]>
aboutcode-org · Jun 17, 2022 · 5bf23bd · 5bf23bd
1 parent 14d3b71
commit 5bf23bd
Show file tree

Hide file tree

Showing 10 changed files with 70 additions and 39 deletions.
diff --git a/scanpipe/migrations/0017_alter_discoveredpackage_package_uid_and_more.py b/scanpipe/migrations/0017_alter_discoveredpackage_package_uid_and_more.py
@@ -0,0 +1,22 @@
+# Generated by Django 4.0.5 on 2022-06-17 06:58
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('scanpipe', '0016_discoveredpackage_package_uid'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='discoveredpackage',
+            name='package_uid',
+            field=models.CharField(blank=True, db_index=True, help_text='Unique identifier for this package.', max_length=1024),
+        ),
+        migrations.AddConstraint(
+            model_name='discoveredpackage',
+            constraint=models.UniqueConstraint(condition=models.Q(('package_uid', ''), _negated=True), fields=('project', 'package_uid'), name='scanpipe_discoveredpackage_unique_package_uid_within_project'),
+        ),
+    ]
diff --git a/scanpipe/models.py b/scanpipe/models.py
@@ -1729,6 +1729,7 @@ class DiscoveredPackage(
     package_uid = models.CharField(
         max_length=1024,
         blank=True,
+        db_index=True,
         help_text=_("Unique identifier for this package."),
     )
 
@@ -1740,6 +1741,13 @@ class DiscoveredPackage(
 
     class Meta:
         ordering = ["uuid"]
+        constraints = [
+            models.UniqueConstraint(
+                fields=["project", "package_uid"],
+                condition=~Q(package_uid=""),
+                name="%(app_label)s_%(class)s_unique_package_uid_within_project",
+            ),
+        ]
 
     def __str__(self):
         return self.package_url or str(self.uuid)
@@ -1807,7 +1815,7 @@ def create_from_data(cls, project, package_data):
         discovered_package.save(save_error=False, capture_exception=False)
         return discovered_package
 
-    def update_from_data(self, package_data):
+    def update_from_data(self, package_data, override=False):
         """
         Update this discovered package instance with the provided `package_data`.
         The `save()` is called only if at least one field was modified.
@@ -1825,11 +1833,9 @@ def update_from_data(self, package_data):
                 continue
 
             current_value = getattr(self, field_name, None)
-            if not current_value:
+            if not current_value or (current_value != value and override):
                 setattr(self, field_name, value)
                 updated_fields.append(field_name)
-            elif current_value != value:
-                pass  # TODO: handle this case
 
         if updated_fields:
             self.save()

diff --git a/scanpipe/pipes/__init__.py b/scanpipe/pipes/__init__.py
@@ -74,22 +74,18 @@ def update_or_create_package(project, package_data, codebase_resource=None):
     DiscoveredPackage using its Package URL and package_uid as a unique key.
     """
     purl_data = DiscoveredPackage.extract_purl_data(package_data)
-    package_uid = package_data.get("package_uid")
-    purl_data_and_package_uid = {
-        **purl_data,
-        "package_uid": package_uid,
-    }
 
     try:
         package = DiscoveredPackage.objects.get(
-            project=project, **purl_data_and_package_uid
+            project=project,
+            package_uid=package_data.get("package_uid"),
+            **purl_data,
         )
     except DiscoveredPackage.DoesNotExist:
         package = None
 
     if package:
         package.update_from_data(package_data)
-
     else:
         if codebase_resource:
             package = codebase_resource.create_and_add_package(package_data)

diff --git a/scanpipe/pipes/scancode.py b/scanpipe/pipes/scancode.py
@@ -43,7 +43,6 @@
 
 from scanpipe import pipes
 from scanpipe.models import CodebaseResource
-from scanpipe.models import DiscoveredPackage
 
 logger = logging.getLogger("scanpipe.pipes")
 
@@ -392,13 +391,14 @@ def create_codebase_resources(project, scanned_codebase):
             defaults=resource_data,
         )
 
-        # Associate DiscoveredPackage to CodebaseResource, if applicable
-        if hasattr(scanned_resource, "for_packages"):
-            for package_uid in scanned_resource.for_packages:
-                package = DiscoveredPackage.objects.get(package_uid=package_uid)
-                set_codebase_resource_for_package(
-                    codebase_resource=codebase_resource, discovered_package=package
-                )
+        for_packages = getattr(scanned_resource, "for_packages", [])
+        for package_uid in for_packages:
+            logger.debug(f"Assign {package_uid} to {codebase_resource}")
+            package = project.discoveredpackages.get(package_uid=package_uid)
+            set_codebase_resource_for_package(
+                codebase_resource=codebase_resource,
+                discovered_package=package,
+            )
 
 
 def create_discovered_packages(project, scanned_codebase):

diff --git a/scanpipe/templates/scanpipe/package_list.html b/scanpipe/templates/scanpipe/package_list.html
@@ -27,7 +27,7 @@
         <tbody>
           {% for package in object_list %}
             <tr class="break-word">
-              <td style="min-width: 500px;">
+              <td style="min-width: 500px;" title="{{ package.package_uid }}">
                 {{ package.package_url }}
               </td>
               <td style="min-width: 300px; max-width: 400px;">

diff --git a/scanpipe/tests/data/is-npm-1.0.0_scan_package.json b/scanpipe/tests/data/is-npm-1.0.0_scan_package.json
@@ -20,7 +20,7 @@
       "errors": [],
       "warnings": [],
       "extra_data": {
-        "spdx_license_list_version": "3.16",
+        "spdx_license_list_version": "3.17",
         "files_count": 3
       }
     }
@@ -131,7 +131,6 @@
       "md5": null,
       "sha256": null,
       "mime_type": null,
-      "file_type": null,
       "programming_language": null,
       "is_binary": false,
       "is_text": false,
@@ -170,7 +169,6 @@
       "md5": null,
       "sha256": null,
       "mime_type": null,
-      "file_type": null,
       "programming_language": null,
       "is_binary": false,
       "is_text": false,
@@ -209,7 +207,6 @@
       "md5": "bc4b18b0c8c32b94883d6fc1d675e919",
       "sha256": "4044efe5626e2fbc40d3d7ce8b263b831d7644ac179e20cdf15b2794f8934030",
       "mime_type": "text/plain",
-      "file_type": "ASCII text",
       "programming_language": "JavaScript",
       "is_binary": false,
       "is_text": true,
@@ -250,7 +247,6 @@
       "md5": "c843e88ecb274d5d573c71be330bff8b",
       "sha256": "522879426298e078881de533b9b3a82fa3bece1336a84bddacf4620008b5d0a3",
       "mime_type": "application/json",
-      "file_type": "JSON data",
       "programming_language": null,
       "is_binary": false,
       "is_text": true,
@@ -408,7 +404,6 @@
       "md5": "a743e0abf08c28a37ecc4bef4dc02f8c",
       "sha256": "e98b263545fe62a00c73b57412cbdbcd0792e013a76f9c88342623ab5e8467c1",
       "mime_type": "text/plain",
-      "file_type": "UTF-8 Unicode text",
       "programming_language": null,
       "is_binary": false,
       "is_text": true,

diff --git a/scanpipe/tests/data/multiple-is-npm-1.0.0_scan_package.json b/scanpipe/tests/data/multiple-is-npm-1.0.0_scan_package.json
@@ -20,7 +20,7 @@
       "errors": [],
       "warnings": [],
       "extra_data": {
-        "spdx_license_list_version": "3.16",
+        "spdx_license_list_version": "3.17",
         "files_count": 6
       }
     }
@@ -200,7 +200,6 @@
       "md5": null,
       "sha256": null,
       "mime_type": null,
-      "file_type": null,
       "programming_language": null,
       "is_binary": false,
       "is_text": false,
@@ -239,7 +238,6 @@
       "md5": null,
       "sha256": null,
       "mime_type": null,
-      "file_type": null,
       "programming_language": null,
       "is_binary": false,
       "is_text": false,
@@ -278,7 +276,6 @@
       "md5": "bc4b18b0c8c32b94883d6fc1d675e919",
       "sha256": "4044efe5626e2fbc40d3d7ce8b263b831d7644ac179e20cdf15b2794f8934030",
       "mime_type": "text/plain",
-      "file_type": "ASCII text",
       "programming_language": "JavaScript",
       "is_binary": false,
       "is_text": true,
@@ -319,7 +316,6 @@
       "md5": "c843e88ecb274d5d573c71be330bff8b",
       "sha256": "522879426298e078881de533b9b3a82fa3bece1336a84bddacf4620008b5d0a3",
       "mime_type": "application/json",
-      "file_type": "JSON data",
       "programming_language": null,
       "is_binary": false,
       "is_text": true,
@@ -477,7 +473,6 @@
       "md5": "a743e0abf08c28a37ecc4bef4dc02f8c",
       "sha256": "e98b263545fe62a00c73b57412cbdbcd0792e013a76f9c88342623ab5e8467c1",
       "mime_type": "text/plain",
-      "file_type": "UTF-8 Unicode text",
       "programming_language": null,
       "is_binary": false,
       "is_text": true,
@@ -593,7 +588,6 @@
       "md5": null,
       "sha256": null,
       "mime_type": null,
-      "file_type": null,
       "programming_language": null,
       "is_binary": false,
       "is_text": false,
@@ -632,7 +626,6 @@
       "md5": null,
       "sha256": null,
       "mime_type": null,
-      "file_type": null,
       "programming_language": null,
       "is_binary": false,
       "is_text": false,
@@ -671,7 +664,6 @@
       "md5": "bc4b18b0c8c32b94883d6fc1d675e919",
       "sha256": "4044efe5626e2fbc40d3d7ce8b263b831d7644ac179e20cdf15b2794f8934030",
       "mime_type": "text/plain",
-      "file_type": "ASCII text",
       "programming_language": "JavaScript",
       "is_binary": false,
       "is_text": true,
@@ -712,7 +704,6 @@
       "md5": "c843e88ecb274d5d573c71be330bff8b",
       "sha256": "522879426298e078881de533b9b3a82fa3bece1336a84bddacf4620008b5d0a3",
       "mime_type": "application/json",
-      "file_type": "JSON data",
       "programming_language": null,
       "is_binary": false,
       "is_text": true,
@@ -870,7 +861,6 @@
       "md5": "a743e0abf08c28a37ecc4bef4dc02f8c",
       "sha256": "e98b263545fe62a00c73b57412cbdbcd0792e013a76f9c88342623ab5e8467c1",
       "mime_type": "text/plain",
-      "file_type": "UTF-8 Unicode text",
       "programming_language": null,
       "is_binary": false,
       "is_text": true,

diff --git a/scanpipe/tests/test_models.py b/scanpipe/tests/test_models.py
@@ -35,6 +35,7 @@
 from django.core.files.uploadedfile import SimpleUploadedFile
 from django.core.management import call_command
 from django.db import DataError
+from django.db import IntegrityError
 from django.db import connection
 from django.test import TestCase
 from django.test import TransactionTestCase
@@ -50,7 +51,6 @@
 from scanpipe.models import ProjectError
 from scanpipe.models import Run
 from scanpipe.models import RunInProgressError
-from scanpipe.models import WebhookSubscription
 from scanpipe.models import get_project_work_directory
 from scanpipe.pipes.fetch import Download
 from scanpipe.pipes.input import copy_input
@@ -1261,6 +1261,10 @@ def test_scanpipe_discovered_package_model_update_from_data(self):
         # Already a value, not updated
         self.assertEqual(package_data1["description"], package.description)
 
+        updated_fields = package.update_from_data(new_data, override=True)
+        self.assertEqual(["description"], updated_fields)
+        self.assertEqual(new_data["description"], package.description)
+
     def test_scanpipe_model_create_user_creates_auth_token(self):
         basic_user = User.objects.create_user(username="basic_user")
         self.assertTrue(basic_user.auth_token.key)
@@ -1412,6 +1416,23 @@ def test_scanpipe_discovered_package_model_create_from_data(self):
         self.assertEqual(package_count, DiscoveredPackage.objects.count())
         self.assertEqual(project_error_count, ProjectError.objects.count())
 
+    def test_scanpipe_discovered_package_model_unique_package_uid_in_project(self):
+        project1 = Project.objects.create(name="Analysis")
+
+        self.assertTrue(package_data1["package_uid"])
+        package = DiscoveredPackage.create_from_data(project1, package_data1)
+        self.assertTrue(package.package_uid)
+
+        with self.assertRaises(IntegrityError):
+            DiscoveredPackage.create_from_data(project1, package_data1)
+
+        package_data_no_uid = package_data1.copy()
+        package_data_no_uid.pop("package_uid")
+        package2 = DiscoveredPackage.create_from_data(project1, package_data_no_uid)
+        self.assertFalse(package2.package_uid)
+        package3 = DiscoveredPackage.create_from_data(project1, package_data_no_uid)
+        self.assertFalse(package3.package_uid)
+
     @skipIf(connection.vendor == "sqlite", "No max_length constraints on SQLite.")
     def test_scanpipe_codebase_resource_create_and_add_package_errors(self):
         project1 = Project.objects.create(name="Analysis")

diff --git a/scanpipe/tests/test_pipes.py b/scanpipe/tests/test_pipes.py
@@ -1179,6 +1179,7 @@ def test_scanpipe_pipes_update_or_create_package(self):
         resource1 = CodebaseResource.objects.create(project=p1, path="filename.ext")
         package_data2 = dict(package_data1)
         package_data2["name"] = "new name"
+        package_data2["package_uid"] = ""
         package2 = update_or_create_package(p1, package_data2, resource1)
         self.assertNotEqual(package.pk, package2.pk)
         self.assertIn(resource1, package2.codebase_resources.all())

diff --git a/setup.cfg b/setup.cfg
@@ -70,7 +70,7 @@ install_requires =
     # Docker
     container_inspector==31.0.0
     # ScanCode-toolkit
-    scancode-toolkit[packages]==31.0.0rc1
+    scancode-toolkit[packages]==31.0.0rc2
     extractcode[full]==31.0.0
     commoncode==31.0.0b4
     # FetchCode