From 2326535cba58c693ddc12ba406d3ba9d6e7076f6 Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Mon, 16 Jun 2025 14:38:34 +0000 Subject: [PATCH 1/2] Replicating changes to error handling from ims-api #176 --- object_storage_api/core/custom_object_id.py | 23 ++++++++++-- object_storage_api/core/exceptions.py | 31 +++++++++------- object_storage_api/repositories/attachment.py | 37 ++++++++----------- object_storage_api/repositories/image.py | 29 +++++---------- 4 files changed, 62 insertions(+), 58 deletions(-) diff --git a/object_storage_api/core/custom_object_id.py b/object_storage_api/core/custom_object_id.py index 12559b9c..036e1364 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,31 @@ 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): """ 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. :raises InvalidObjectIdError: If the string value is an invalid `ObjectId`. """ + response_detail = 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 122afd7f..b41eb98a 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 fa273c14..593fc90c 100644 --- a/object_storage_api/repositories/attachment.py +++ b/object_storage_api/repositories/attachment.py @@ -64,8 +64,10 @@ 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) + attachment = self._attachments_collection.find_one( + {"_id": CustomObjectId(attachment_id, entity_type="attachment", not_found_if_invalid=True)}, + session=session, + ) except InvalidObjectIdError as exc: exc.status_code = 404 exc.response_detail = "Attachment not found" @@ -73,7 +75,7 @@ def get(self, attachment_id: str, session: Optional[ClientSession] = None) -> Op 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 +123,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 +147,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 +163,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 5c4c2e73..de85b95c 100644 --- a/object_storage_api/repositories/image.py +++ b/object_storage_api/repositories/image.py @@ -61,15 +61,16 @@ def get(self, image_id: str, session: Optional[ClientSession] = None) -> ImageOu """ 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) + image = self._images_collection.find_one( + {"_id": CustomObjectId(image_id, entity_type="image", not_found_if_invalid=True)}, session=session + ) except InvalidObjectIdError as exc: exc.status_code = 404 exc.response_detail = "Image not found" raise exc 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 +125,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 +161,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 +176,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. From a9ccb21aa990e91bf3619a4f986c459f45712c43 Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Wed, 18 Jun 2025 13:10:51 +0000 Subject: [PATCH 2/2] Clean up of unnecessary excepts #176 --- object_storage_api/core/custom_object_id.py | 18 +++++++++++++++--- object_storage_api/repositories/attachment.py | 14 ++++---------- object_storage_api/repositories/image.py | 12 ++++-------- object_storage_api/services/attachment.py | 9 ++------- object_storage_api/services/image.py | 10 +++------- 5 files changed, 28 insertions(+), 35 deletions(-) diff --git a/object_storage_api/core/custom_object_id.py b/object_storage_api/core/custom_object_id.py index 036e1364..961694cb 100644 --- a/object_storage_api/core/custom_object_id.py +++ b/object_storage_api/core/custom_object_id.py @@ -20,17 +20,29 @@ class CustomObjectId(ObjectId): purpose of handling MongoDB `_id` fields that are of type `ObjectId`. """ - def __init__(self, value: str, entity_type: Optional[str] = None, not_found_if_invalid: bool = False): + 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. + 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 = None if entity_type is None else f"{entity_type.capitalize()} not found" + 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): diff --git a/object_storage_api/repositories/attachment.py b/object_storage_api/repositories/attachment.py index 593fc90c..68db6fa4 100644 --- a/object_storage_api/repositories/attachment.py +++ b/object_storage_api/repositories/attachment.py @@ -62,16 +62,10 @@ 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 = self._attachments_collection.find_one( - {"_id": CustomObjectId(attachment_id, entity_type="attachment", not_found_if_invalid=True)}, - 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) diff --git a/object_storage_api/repositories/image.py b/object_storage_api/repositories/image.py index de85b95c..a4c80717 100644 --- a/object_storage_api/repositories/image.py +++ b/object_storage_api/repositories/image.py @@ -60,14 +60,10 @@ 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 = self._images_collection.find_one( - {"_id": CustomObjectId(image_id, entity_type="image", not_found_if_invalid=True)}, 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(entity_id=image_id, entity_type="image") diff --git a/object_storage_api/services/attachment.py b/object_storage_api/services/attachment.py index f3dc9cfb..b048e334 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 2636e1e7..c2f2a697 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: