feat(UX): Improve some custom library upload error messages (#3318)

* feat(UX): Improve some custom library upload error messages

* Fix tests

* Add load method error support

* Update views.py

* fix type annotations, adapt all calls

---------

Co-authored-by: eric-intuitem <71850047+eric-intuitem@users.noreply.github.com>

Author: monsieurswag

backend/core/models.py

@@ -4,7 +4,7 @@
 import hashlib
 from datetime import date, datetime
 from pathlib import Path
-from typing import Self, Union, List, Optional, Literal
+from typing import Self, Union, List, Optional, Literal, Tuple
 import statistics
 
 from django.contrib.contenttypes.models import ContentType
@@ -306,11 +306,11 @@ def __init_class__(cls):
     @classmethod
     def store_library_content(
         cls, library_content: bytes, builtin: bool = False, dry_run: bool = False
-    ) -> Union["StoredLibrary", dict, None]:
+    ) -> Tuple[Optional[Union["StoredLibrary", dict]], Optional[str]]:
         hash_checksum = sha256(library_content)
         if not dry_run and hash_checksum in StoredLibrary.HASH_CHECKSUM_SET:
             # We do not store the library if its hash checksum is in the database.
-            return None
+            return None, "libraryAlreadyLoadedError"
         try:
             library_data = yaml.safe_load(library_content)
             if not isinstance(library_data, dict):
@@ -344,21 +344,24 @@ def store_library_content(
                 key: (1 if key == "framework" else len(value))
                 for key, value in library_data["objects"].items()
             }
-            return {
-                "name": library_data["name"],
-                "urn": urn,
-                "locale": locale,
-                "version": version,
-                "ref_id": library_data.get("ref_id"),
-                "description": library_data.get("description"),
-                "provider": library_data.get("provider"),
-                "packager": library_data.get("packager"),
-                "publication_date": library_data.get("publication_date"),
-                "objects_meta": objects_meta,
-                "is_loaded": is_loaded,
-                "builtin": builtin,
-                "copyright": library_data.get("copyright"),
-            }
+            return (
+                {
+                    "name": library_data["name"],
+                    "urn": urn,
+                    "locale": locale,
+                    "version": version,
+                    "ref_id": library_data.get("ref_id"),
+                    "description": library_data.get("description"),
+                    "provider": library_data.get("provider"),
+                    "packager": library_data.get("packager"),
+                    "publication_date": library_data.get("publication_date"),
+                    "objects_meta": objects_meta,
+                    "is_loaded": is_loaded,
+                    "builtin": builtin,
+                    "copyright": library_data.get("copyright"),
+                },
+                None,
+            )
 
         same_version_lib = StoredLibrary.objects.filter(
             urn=urn, locale=locale, version=version
@@ -368,10 +371,13 @@ def store_library_content(
             logger.info("update hash", urn=urn)
             same_version_lib.hash_checksum = hash_checksum
             same_version_lib.save()
-            return None
+            return None, "libraryAlreadyLoadedError"
 
         if StoredLibrary.objects.filter(urn=urn, locale=locale, version__gte=version):
-            return None  # We do not accept to store outdated libraries
+            return (
+                None,
+                "libraryOutdatedError",
+            )  # We do not accept to store outdated libraries
 
         with transaction.atomic():
             # This code allows adding outdated libraries in the library store but they will be erased if a greater version of this library is stored.
@@ -425,16 +431,16 @@ def store_library_content(
                 ),  # autoload is true if the library contains requirement mapping sets
             )
             new_library.filtering_labels.set(filtering_labels)
-            return new_library
+            return new_library, None
 
     @classmethod
     def store_library_file(
         cls, fname: Path, builtin: bool = False
-    ) -> "StoredLibrary | None":
+    ) -> Tuple[Optional["StoredLibrary"], Optional[str]]:
         with open(fname, "rb") as f:
             library_content = f.read()
 
-        return StoredLibrary.store_library_content(library_content, builtin)
+        return StoredLibrary.store_library_content(library_content, builtin=builtin)
 
     def get_loaded_library(self) -> Optional["LoadedLibrary"]:
         if not self.is_loaded:

backend/library/management/commands/storelibraries.py

@@ -31,13 +31,19 @@ def handle(self, *args, **options):
         for fname in library_files:
             # logger.info("Begin library file storage", filename=fname)
             try:
-                library = StoredLibrary.store_library_file(fname, True)
+                library, error = StoredLibrary.store_library_file(fname, True)
                 if library:
                     logger.info(
                         "Successfully stored library",
                         filename=fname,
                         library=library,
                     )
+                elif error:
+                    logger.warning(
+                        "Library skipped",
+                        filename=fname,
+                        reason=error,
+                    )
             except Exception:
                 logger.error("Invalid library file", filename=fname)
 

backend/library/tests/test_store_library_content.py

@@ -140,25 +140,26 @@
 class TestStoreLibraryContent:
     @pytest.mark.django_db
     def test_store_library_content_sets_autoload_to_true_if_lib_has_mappings(self):
-        stored_library = StoredLibrary.store_library_content(
+        stored_library, _ = StoredLibrary.store_library_content(
             SAMPLE_YAML_LIB_WITH_MAPPINGS
         )
         assert stored_library is not None
         assert stored_library.autoload is True
 
     @pytest.mark.django_db
     def test_store_library_content_sets_autoload_to_false_if_lib_has_no_mappings(self):
-        stored_library = StoredLibrary.store_library_content(
+        stored_library, _ = StoredLibrary.store_library_content(
             SAMPLE_YAML_LIB_NO_MAPPINGS
         )
         assert stored_library is not None
         assert stored_library.autoload is False
 
     @pytest.mark.django_db
     def test_store_library_content_dry_run(self):
-        library_data = StoredLibrary.store_library_content(
+        library_data, error = StoredLibrary.store_library_content(
             SAMPLE_YAML_LIB_NO_MAPPINGS, dry_run=True
         )
+        assert error is None
         assert isinstance(library_data, dict)
         assert library_data["urn"] == "urn:intuitem:test:library:nist-csf-1.1"
         assert library_data["version"] == 5

backend/library/views.py

@@ -446,7 +446,15 @@ def upload_library(self, request):
                 dry_run = request.query_params.get("dry_run", "false").lower() == "true"
 
                 # Store the library content (YAML bytes)
-                library = StoredLibrary.store_library_content(content, dry_run=dry_run)
+                library, error = StoredLibrary.store_library_content(
+                    content, dry_run=dry_run
+                )
+                if error is not None:
+                    return HttpResponse(
+                        json.dumps({"error": error}),
+                        status=HTTP_422_UNPROCESSABLE_ENTITY,
+                    )
+
                 if dry_run:
                     logger.info("Dry run library upload successful")
                     return Response(library)