add documentation to methods TODO: add get check to delete e2e test #38

object_storage_api/core/exceptions.py

@@ -6,6 +6,9 @@
 # TODO: Some of this file is identical to the one in inventory-management-system-api - Use common repo?
 
 
+from typing import Optional
+
+
 class BaseAPIException(Exception):
     """
     Base exception for API errors.
@@ -19,7 +22,7 @@ class BaseAPIException(Exception):
 
     detail: str
 
-    def __init__(self, detail: str, response_detail: str = None):
+    def __init__(self, detail: str, response_detail: Optional[str] = None):
         """
         Initialise the exception.
 
@@ -67,13 +70,13 @@ class MissingRecordError(DatabaseError):
     status_code = 404
     response_detail = "Requested record was not found"
 
-    def __init__(self, detail: str, entity_name: str = None):
+    def __init__(self, detail: str, entity_name: Optional[str] = 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 entity_name: Name of the entity to include in the response.
+        :param entity_name: Name of the entity to include in the response detail.
         """
         super().__init__(detail)
 

object_storage_api/repositories/image.py

@@ -85,13 +85,24 @@ def list(self, entity_id: Optional[str], primary: Optional[bool], session: Clien
         return [ImageOut(**image) for image in images]
 
     def delete(self, image_id: str, session: ClientSession = None) -> str:
+        """
+        Delete an image by its ID from a MongoDB database.
+
+        :param image_id: The ID of the image to delete.
+        :param session: PyMongo ClientSession to use for database operations
+        :return: Deleted image if found.
+        :raises MissingRecordError: If the supplied `image_id` is non-existent.
+        :raises InvalidObjectIdError: If the supplied `image_id` is invalid.
+        """
+        logger.info("Deleting image with ID: %s from the database", image_id)
         try:
-            new_image_id = CustomObjectId(image_id)
-        except InvalidObjectIdError as e:
-            exc = MissingRecordError(f"Invalid image_id given: {image_id}", "image")
-            raise exc from e
+            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.find_one_and_delete(
-            filter={"_id": new_image_id}, projection={"object_key": True}, session=session
+            filter={"_id": image_id}, projection={"object_key": True}, session=session
         )
         if response is None:
             exc = MissingRecordError(f"Requested Image was not found: {image_id}", "image")

object_storage_api/routers/image.py

@@ -69,11 +69,11 @@ def get_images(
     return image_service.list(entity_id, primary)
 
 
-@router.delete(
-    path="/{image_id}", summary="Delete an image by its ID", response_description="Successfully deleted image"
-)
+@router.delete(path="/{image_id}", summary="Delete an image by ID", response_description="Image deleted Sucessfully")
 def delete_image(
+    image_id: Annotated[str, Path(description="The ID of the image to delete")],
     image_service: ImageServiceDep,
-    image_id: Annotated[str, Path(description="The ID of the image that is to be deleted")],
 ) -> None:
+    # pylint: disable=missing-function-docstring
+    logger.info("Deleting image with ID: %s", image_id)
     image_service.delete(image_id)

object_storage_api/services/image.py

@@ -87,5 +87,11 @@ def list(self, entity_id: Optional[str] = None, primary: Optional[bool] = None)
         return [ImageSchema(**image.model_dump()) for image in images]
 
     def delete(self, image_id: str) -> None:
+        """
+        Delete an image by its ID.
+
+        :param image_id: The ID of the image to delete.
+        """
+
         object_key = self._image_repository.delete(image_id)
         self._image_store.delete(object_key)

object_storage_api/stores/image.py

@@ -39,6 +39,13 @@ def upload(self, image_id: str, image_metadata: ImagePostMetadataSchema, upload_
         return object_key
 
     def delete(self, object_key: str) -> None:
+        """
+        Deletes a given image from object storage.
+
+        :param object_key: Key of the image to delete.
+        """
+
+        logger.info("Deleting image file from the object storage")
         s3_client.delete_object(
             Bucket=object_storage_config.bucket_name.get_secret_value(),
             Key=object_key,

test/e2e/test_image.py

@@ -281,7 +281,7 @@ def check_delete_image_success(self) -> None:
 
         assert self._delete_response_image.status_code == 200
 
-    def check_delete_item_failed_with_detail(self, status_code: int, detail: str) -> None:
+    def check_delete_item_failed_with_detail(self) -> None:
         """
         Checks that a prior call to `delete_image` gave a failed response with the expected code and
         error message.
@@ -290,8 +290,8 @@ def check_delete_item_failed_with_detail(self, status_code: int, detail: str) ->
         :param detail: Expected detail given in the response.
         """
 
-        assert self._delete_response_image.status_code == status_code
-        assert self._delete_response_image.json()["detail"] == detail
+        assert self._delete_response_image.status_code == 404
+        assert self._delete_response_image.json()["detail"] == "Image not found"
 
 
 class TestDelete(DeleteDSL):
@@ -309,10 +309,10 @@ def test_delete_with_non_existent_id(self):
         """Test deleting a non-existent item."""
 
         self.delete_image(str(ObjectId()))
-        self.check_delete_item_failed_with_detail(404, "Image not found")
+        self.check_delete_item_failed_with_detail()
 
     def test_delete_with_invalid_id(self):
         """Test deleting an item with an invalid ID."""
 
         self.delete_image("invalid_id")
-        self.check_delete_item_failed_with_detail(404, "Image not found")
+        self.check_delete_item_failed_with_detail()

test/unit/repositories/conftest.py

@@ -78,12 +78,13 @@ def mock_find(collection_mock: Mock, documents: List[dict]) -> None:
         cursor_mock.__iter__.return_value = iter(documents)
         collection_mock.find.return_value = cursor_mock
 
+    @staticmethod
     def mock_find_one_and_delete(collection_mock: Mock, document: dict | None) -> None:
         """
-        Mocks the `find_one` method of the MongoDB database collection mock to return a specific document.
+        Mocks the `find_one_and_delete` method of the MongoDB database collection mock to return a specific document.
 
         :param collection_mock: Mocked MongoDB database collection instance.
-        :param document: The document to be returned by the `find_one` method.
+        :param document: The document to be returned by the `find_one_and_delete` method.
         """
         if collection_mock.find_one_and_delete.side_effect is None:
             collection_mock.find_one_and_delete.side_effect = [document]

test/unit/repositories/test_image.py

@@ -10,7 +10,7 @@
 import pytest
 from bson import ObjectId
 
-from object_storage_api.core.exceptions import MissingRecordError
+from object_storage_api.core.exceptions import InvalidObjectIdError, MissingRecordError
 from object_storage_api.models.image import ImageIn, ImageOut
 from object_storage_api.repositories.image import ImageRepo
 
@@ -48,7 +48,7 @@ def mock_create(
         """
         Mocks database methods appropriately to test the `create` repo method.
 
-        :param image_in_data: Dictionary containing the image data as would be required for a `ImageIn`
+        :param image_in_data: Dictionary containing the image data as would be required for an `ImageIn`
                                    database model (i.e. no created and modified times required).
         """
 
@@ -102,7 +102,7 @@ def mock_list(self, image_in_data: list[dict]) -> None:
         Mocks database methods appropriately to test the `list` repo method.
 
         :param image_in_data: List of dictionaries containing the image data as would be required for an
-            `ImageIn` database model (i.e. no ID or created and modified times required).
+            `ImageIn` database model (i.e. no created and modified times required).
         """
         self._expected_image_out = [
             ImageOut(**ImageIn(**image_in_data).model_dump()) for image_in_data in image_in_data
@@ -184,7 +184,9 @@ def mock_delete(self, image_id: str, image_in_data: dict = None) -> None:
         """
         Mocks database methods appropriately to test the `delete` repo method.
 
-        :param deleted_count: Number of documents deleted successfully.
+        :param image_id: ID of the image to delete.
+        :param image_in_data: Dictionary containing the image data as would be required for an `ImageIn`
+                                   database model (i.e. no created and modified times required).
         """
         if image_in_data:
             image_in_data["id"] = image_id
@@ -224,15 +226,15 @@ def check_delete_success(self) -> None:
         self.images_collection.find_one_and_delete.assert_called_once_with(
             filter={"_id": ObjectId(self._delete_image_id)}, projection={"object_key": True}, session=self.mock_session
         )
-        self._obtained_object_key = self._expected_object_key
+        assert self._obtained_object_key == self._expected_object_key
 
     def check_delete_failed_with_exception(self, message: str, assert_delete: bool = False) -> None:
         """
         Checks that a prior call to `call_delete_expecting_error` worked as expected, raising an exception
         with the correct message.
 
         :param message: Expected message of the raised exception.
-        :param assert_delete: Whether the `delete_one` method is expected to be called or not.
+        :param assert_delete: Whether the `find_one_and_delete` method is expected to be called or not.
         """
 
         if not assert_delete:
@@ -272,5 +274,5 @@ def test_delete_invalid_id(self):
 
         image_id = "invalid-id"
 
-        self.call_delete_expecting_error(image_id, MissingRecordError)
-        self.check_delete_failed_with_exception(f"Invalid image_id given: {image_id}")
+        self.call_delete_expecting_error(image_id, InvalidObjectIdError)
+        self.check_delete_failed_with_exception(f"Invalid ObjectId value '{image_id}'")

test/unit/services/test_image.py

@@ -227,8 +227,9 @@ def check_delete_success(self) -> None:
 
 
 class TestDelete(DeleteDSL):
-    """Tests for deleting images."""
+    """Tests for deleting an image."""
 
     def test_delete(self):
+        """Test for deleting an image."""
         self.call_delete("test_image_id")
         self.check_delete_success()

test/unit/stores/test_image.py

@@ -102,10 +102,10 @@ def check_delete_success(self) -> None:
 
 
 class TestDelete(DeleteDSL):
-    """Tests for creating a presigned url for an image."""
+    """Tests for deleting an image from object storage."""
 
     def test_create_presigned_post(self):
-        """Test creating a presigned url for an image."""
+        """Test for deleting an image from object storage."""
 
         self.call_delete()
         self.check_delete_success()