diff --git a/object_storage_api/core/custom_object_id.py b/object_storage_api/core/custom_object_id.py index 12559b9..961694c 100644 --- a/object_storage_api/core/custom_object_id.py +++ b/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) diff --git a/object_storage_api/core/exceptions.py b/object_storage_api/core/exceptions.py index 122afd7..b41eb98 100644 --- a/object_storage_api/core/exceptions.py +++ b/object_storage_api/core/exceptions.py @@ -22,13 +22,14 @@ 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) @@ -36,6 +37,11 @@ def __init__(self, detail: str, response_detail: Optional[str] = None): 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): diff --git a/object_storage_api/repositories/attachment.py b/object_storage_api/repositories/attachment.py index fa273c1..68db6fa 100644 --- a/object_storage_api/repositories/attachment.py +++ b/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. diff --git a/object_storage_api/repositories/image.py b/object_storage_api/repositories/image.py index 5c4c2e7..a4c8071 100644 --- a/object_storage_api/repositories/image.py +++ b/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. diff --git a/object_storage_api/services/attachment.py b/object_storage_api/services/attachment.py index f3dc9cf..b048e33 100644 --- a/object_storage_api/services/attachment.py +++ b/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: diff --git a/object_storage_api/services/image.py b/object_storage_api/services/image.py index 2636e1e..c2f2a69 100644 --- a/object_storage_api/services/image.py +++ b/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: