Replicating changes to error handling from ims-api #176

object_storage_api/core/custom_object_id.py

@@ -6,7 +6,10 @@
 # TODO: This file is identical to the one in inventory-management-system-api - Use common repo?
 
 
+from typing import Optional
+
 from bson import ObjectId
+from fastapi import status
 
 from object_storage_api.core.exceptions import InvalidObjectIdError
 
@@ -17,17 +20,43 @@ class CustomObjectId(ObjectId):
     purpose of handling MongoDB `_id` fields that are of type `ObjectId`.
     """
 
-    def __init__(self, value: str):
+    def __init__(
+        self,
+        value: str,
+        entity_type: Optional[str] = None,
+        not_found_if_invalid: bool = False,
+        response_detail: Optional[str] = None,
+    ):
         """
         Construct a `CustomObjectId` from a string.
 
         :param value: The string value to be validated, representing the `ObjectId`.
+        :param entity_type: Name of the entity type e.g. catalogue categories/systems (Used for logging).
+        :param not_found_if_invalid: Whether an error due to an invalid ID should be raised as a not found error
+                                     or not. Unprocessable entity is used if left as the default value False.
+        :param response_detail: Response detail to return in the the request if uncaught, overrides any error message
+                                constructed by the other parameters.
         :raises InvalidObjectIdError: If the string value is an invalid `ObjectId`.
         """
+        response_detail = (
+            response_detail
+            if response_detail is not None
+            else (None if entity_type is None else f"{entity_type.capitalize()} not found")
+        )
+        status_code = status.HTTP_404_NOT_FOUND if not_found_if_invalid else None
+
         if not isinstance(value, str):
-            raise InvalidObjectIdError(f"ObjectId value '{value}' must be a string")
+            raise InvalidObjectIdError(
+                f"ObjectId value '{value}' must be a string",
+                response_detail=response_detail,
+                status_code=status_code,
+            )
 
         if not ObjectId.is_valid(value):
-            raise InvalidObjectIdError(f"Invalid ObjectId value '{value}'")
+            raise InvalidObjectIdError(
+                f"Invalid ObjectId value '{value}'",
+                response_detail=response_detail,
+                status_code=status_code,
+            )
 
         super().__init__(value)

object_storage_api/core/exceptions.py

@@ -22,20 +22,26 @@ class BaseAPIException(Exception):
 
     detail: str
 
-    def __init__(self, detail: str, response_detail: Optional[str] = None):
+    def __init__(self, detail: str, response_detail: Optional[str] = None, status_code: Optional[int] = None):
         """
         Initialise the exception.
 
         :param detail: Specific detail of the exception (just like Exception would take - this will only be logged
                        and not returned in a response).
-        :param response_detail: Generic detail of the exception that will be returned in a response.
+        :param response_detail: Generic detail of the exception that will be returned in a response if left uncaught.
+        :param status_code: Status code that will be returned in a response.
         """
         super().__init__(detail)
 
         self.detail = detail
 
         if response_detail is not None:
             self.response_detail = response_detail
+        # If there is no response detail defined just use the detail
+        elif not hasattr(self, "response_detail"):
+            self.response_detail = detail
+        if status_code is not None:
+            self.status_code = status_code
 
 
 class DatabaseError(BaseAPIException):
@@ -93,22 +99,19 @@ def __init__(self, detail: str, entity_type: str):
 class MissingRecordError(DatabaseError):
     """A specific database record was requested but could not be found."""
 
-    status_code = status.HTTP_404_NOT_FOUND
-    response_detail = "Requested record was not found"
-
-    def __init__(self, detail: str, response_detail: Optional[str] = None, entity_type: Optional[str] = None):
+    def __init__(self, entity_id: str, entity_type: str, use_422=False):
         """
         Initialise the exception.
 
-        :param detail: Specific detail of the exception (just like Exception would take - this will only be logged
-                       and not returned in a response).
-        :param response_detail: Generic detail of the exception to be returned in the response.
-        :param entity_type: Name of the entity to include in the response detail.
+        :param entity_id: ID of the record that was found to be missing.
+        :param entity_type: Name of the entity type e.g. catalogue categories/systems (Used for logging).
+        :param use_422: Whether the error returned if uncaught should be a 422 (default is 404 when false).
         """
-        super().__init__(detail, response_detail)
-
-        if entity_type is not None:
-            self.response_detail = f"{entity_type.capitalize()} not found"
+        super().__init__(
+            detail=f"No {entity_type} found with ID: {entity_id}",
+            response_detail=f"{entity_type.capitalize()} not found",
+            status_code=status.HTTP_422_UNPROCESSABLE_ENTITY if use_422 else status.HTTP_404_NOT_FOUND,
+        )
 
 
 class DuplicateRecordError(DatabaseError):

object_storage_api/repositories/attachment.py

@@ -62,18 +62,14 @@ def get(self, attachment_id: str, session: Optional[ClientSession] = None) -> Op
         """
 
         logger.info("Retrieving attachment with ID: %s from the database", attachment_id)
-
-        try:
-            attachment_id = CustomObjectId(attachment_id)
-            attachment = self._attachments_collection.find_one({"_id": attachment_id}, session=session)
-        except InvalidObjectIdError as exc:
-            exc.status_code = 404
-            exc.response_detail = "Attachment not found"
-            raise exc
+        attachment = self._attachments_collection.find_one(
+            {"_id": CustomObjectId(attachment_id, entity_type="attachment", not_found_if_invalid=True)},
+            session=session,
+        )
 
         if attachment:
             return AttachmentOut(**attachment)
-        raise MissingRecordError(detail=f"No attachment found with ID: {attachment_id}", entity_type="attachment")
+        raise MissingRecordError(entity_id=attachment_id, entity_type="attachment")
 
     def list(self, entity_id: Optional[str], session: Optional[ClientSession] = None) -> list[AttachmentOut]:
         """
@@ -121,24 +117,19 @@ def update(self, attachment_id: str, attachment: AttachmentIn, session: ClientSe
         :raises DuplicateRecordError: If a duplicate attachment is found within the parent entity.
         """
 
-        try:
-            attachment_id = CustomObjectId(attachment_id)
-        except InvalidObjectIdError as exc:
-            exc.status_code = 404
-            exc.response_detail = "Attachment not found"
-            raise exc
-
         logger.info("Updating attachment metadata with ID: %s", attachment_id)
         try:
             self._attachments_collection.update_one(
-                {"_id": attachment_id}, {"$set": attachment.model_dump(by_alias=True)}, session=session
+                {"_id": CustomObjectId(attachment_id, entity_type="attachment", not_found_if_invalid=True)},
+                {"$set": attachment.model_dump(by_alias=True)},
+                session=session,
             )
         except pymongo.errors.DuplicateKeyError as exc:
             raise DuplicateRecordError(
                 "Duplicate attachment found within the parent entity", entity_type="attachment"
             ) from exc
 
-        return self.get(attachment_id=str(attachment_id), session=session)
+        return self.get(attachment_id=attachment_id, session=session)
 
     def delete(self, attachment_id: str, session: Optional[ClientSession] = None) -> None:
         """
@@ -150,15 +141,12 @@ def delete(self, attachment_id: str, session: Optional[ClientSession] = None) ->
         :raises InvalidObjectIdError: If the supplied `attachment_id` is invalid.
         """
         logger.info("Deleting attachment with ID: %s from the database", attachment_id)
-        try:
-            attachment_id = CustomObjectId(attachment_id)
-        except InvalidObjectIdError as exc:
-            exc.status_code = 404
-            exc.response_detail = "Attachment not found"
-            raise exc
-        response = self._attachments_collection.delete_one(filter={"_id": attachment_id}, session=session)
+        response = self._attachments_collection.delete_one(
+            filter={"_id": CustomObjectId(attachment_id, entity_type="attachment", not_found_if_invalid=True)},
+            session=session,
+        )
         if response.deleted_count == 0:
-            raise MissingRecordError(f"No attachment found with ID: {attachment_id}", entity_type="attachment")
+            raise MissingRecordError(entity_id=attachment_id, entity_type="attachment")
 
     def delete_by_entity_id(self, entity_id: str, session: Optional[ClientSession] = None) -> None:
         """
@@ -169,9 +157,8 @@ def delete_by_entity_id(self, entity_id: str, session: Optional[ClientSession] =
         """
         logger.info("Deleting attachments with entity ID: %s from the database", entity_id)
         try:
-            entity_id = CustomObjectId(entity_id)
             # Given it is deleting multiple, we are not raising an exception if no attachments were found to be deleted
-            self._attachments_collection.delete_many(filter={"entity_id": entity_id}, session=session)
+            self._attachments_collection.delete_many(filter={"entity_id": CustomObjectId(entity_id)}, session=session)
         except InvalidObjectIdError:
             # As this method takes in an entity_id to delete multiple attachments, and to hide the database behaviour,
             # we treat any invalid entity_id the same as a valid one that has no attachments associated to it.

object_storage_api/repositories/image.py

@@ -60,16 +60,13 @@ def get(self, image_id: str, session: Optional[ClientSession] = None) -> ImageOu
         :raises InvalidObjectIdError: If the supplied `image_id` is invalid.
         """
         logger.info("Retrieving image with ID: %s from the database", image_id)
-        try:
-            image_id = CustomObjectId(image_id)
-            image = self._images_collection.find_one({"_id": image_id}, session=session)
-        except InvalidObjectIdError as exc:
-            exc.status_code = 404
-            exc.response_detail = "Image not found"
-            raise exc
+        image = self._images_collection.find_one(
+            {"_id": CustomObjectId(image_id, entity_type="image", not_found_if_invalid=True)}, session=session
+        )
+
         if image:
             return ImageOut(**image)
-        raise MissingRecordError(detail=f"No image found with ID: {image_id}", entity_type="image")
+        raise MissingRecordError(entity_id=image_id, entity_type="image")
 
     def list(
         self, entity_id: Optional[str], primary: Optional[bool], session: Optional[ClientSession] = None
@@ -124,12 +121,7 @@ def update(self, image_id: str, image: ImageIn, update_primary: bool, session: C
         :raises DuplicateRecordError: If a duplicate attachment is found within the parent entity.
         """
 
-        try:
-            image_id = CustomObjectId(image_id)
-        except InvalidObjectIdError as exc:
-            exc.status_code = 404
-            exc.response_detail = "Image not found"
-            raise exc
+        image_id = CustomObjectId(image_id, entity_type="image", not_found_if_invalid=True)
 
         try:
             if update_primary:
@@ -165,15 +157,11 @@ def delete(self, image_id: str, session: Optional[ClientSession] = None) -> None
         :raises InvalidObjectIdError: If the supplied `image_id` is invalid.
         """
         logger.info("Deleting image with ID: %s from the database", image_id)
-        try:
-            image_id = CustomObjectId(image_id)
-        except InvalidObjectIdError as exc:
-            exc.status_code = 404
-            exc.response_detail = "Image not found"
-            raise exc
-        response = self._images_collection.delete_one(filter={"_id": image_id}, session=session)
+        response = self._images_collection.delete_one(
+            filter={"_id": CustomObjectId(image_id, entity_type="image", not_found_if_invalid=True)}, session=session
+        )
         if response.deleted_count == 0:
-            raise MissingRecordError(f"No image found with ID: {image_id}", entity_type="image")
+            raise MissingRecordError(entity_id=image_id, entity_type="image")
 
     def delete_by_entity_id(self, entity_id: str, session: Optional[ClientSession] = None) -> None:
         """
@@ -184,9 +172,8 @@ def delete_by_entity_id(self, entity_id: str, session: Optional[ClientSession] =
         """
         logger.info("Deleting images with entity ID: %s from the database", entity_id)
         try:
-            entity_id = CustomObjectId(entity_id)
             # Given it is deleting multiple, we are not raising an exception if no images were found to be deleted
-            self._images_collection.delete_many(filter={"entity_id": entity_id}, session=session)
+            self._images_collection.delete_many(filter={"entity_id": CustomObjectId(entity_id)}, session=session)
         except InvalidObjectIdError:
             # As this method takes in an entity_id to delete multiple images, and to hide the database behaviour, we
             # treat any invalid entity_id the same as a valid one that has no images associated to it.

object_storage_api/services/attachment.py

@@ -15,7 +15,6 @@
 from object_storage_api.core.custom_object_id import CustomObjectId
 from object_storage_api.core.exceptions import (
     FileTypeMismatchException,
-    InvalidObjectIdError,
     UnsupportedFileExtensionException,
     UploadLimitReachedError,
 )
@@ -63,12 +62,8 @@ def create(self, attachment: AttachmentPostSchema) -> AttachmentPostResponseSche
         :raises UnsupportedFileExtensionException: If the file extension of the attachment is not supported.
         :raises UploadLimitReachedError: If the upload limit has been reached.
         """
-        try:
-            CustomObjectId(attachment.entity_id)
-        except InvalidObjectIdError as exc:
-            # Provide more specific detail
-            exc.response_detail = "Invalid `entity_id` given"
-            raise exc
+        # Check the entity ID is valid
+        CustomObjectId(attachment.entity_id, response_detail="Invalid `entity_id` given")
 
         file_extension = Path(attachment.file_name).suffix
         if not file_extension or file_extension.lower() not in config.attachment.allowed_file_extensions:

object_storage_api/services/image.py

@@ -15,7 +15,6 @@
 from object_storage_api.core.custom_object_id import CustomObjectId
 from object_storage_api.core.exceptions import (
     FileTypeMismatchException,
-    InvalidObjectIdError,
     UnsupportedFileExtensionException,
     UploadLimitReachedError,
 )
@@ -65,12 +64,9 @@ def create(self, image_metadata: ImagePostMetadataSchema, upload_file: UploadFil
         :raises FileTypeMismatchException: If the extension and content type of the image do not match.
         :raises UploadLimitReachedError: If the upload limit has been reached.
         """
-        try:
-            CustomObjectId(image_metadata.entity_id)
-        except InvalidObjectIdError as exc:
-            # Provide more specific detail
-            exc.response_detail = "Invalid `entity_id` given"
-            raise exc
+
+        # Check the entity ID is valid
+        CustomObjectId(image_metadata.entity_id, response_detail="Invalid `entity_id` given")
 
         file_extension = Path(upload_file.filename).suffix
         if not file_extension or file_extension.lower() not in config.image.allowed_file_extensions: