Merge pull request #72 from ral-facilities/add-delete-endpoint-for-im…

…age-#38

Add delete endpoint for image

object_storage_api/repositories/image.py

@@ -95,3 +95,23 @@ def list(self, entity_id: Optional[str], primary: Optional[bool], session: Clien
 
         images = self._images_collection.find(query, session=session)
         return [ImageOut(**image) for image in images]
+
+    def delete(self, image_id: str, session: ClientSession = None) -> None:
+        """
+        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
+        :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:
+            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)
+        if response.deleted_count == 0:
+            raise MissingRecordError(f"No image found with ID: {image_id}", "image")

object_storage_api/routers/image.py

@@ -78,3 +78,18 @@ def get_image(
     logger.info("Getting image with ID: %s", image_id)
 
     return image_service.get(image_id)
+
+
+@router.delete(
+    path="/{image_id}",
+    summary="Delete an image by ID",
+    response_description="Image deleted successfully",
+    status_code=status.HTTP_204_NO_CONTENT,
+)
+def delete_image(
+    image_id: Annotated[str, Path(description="The ID of the image to delete")],
+    image_service: ImageServiceDep,
+) -> 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

@@ -96,3 +96,14 @@ def list(self, entity_id: Optional[str] = None, primary: Optional[bool] = None)
         """
         images = self._image_repository.list(entity_id, primary)
         return [ImageMetadataSchema(**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.
+        """
+        stored_image = self._image_repository.get(image_id)
+        # Deletes image from object store first to prevent unreferenced objects in storage
+        self._image_store.delete(stored_image.object_key)
+        self._image_repository.delete(image_id)

object_storage_api/stores/attachment.py

@@ -31,7 +31,10 @@ def create_presigned_post(
         """
         object_key = f"attachments/{attachment.entity_id}/{attachment_id}"
 
-        logger.info("Generating a presigned URL for uploading the attachment")
+        logger.info(
+            "Generating a presigned URL for uploading the attachment with object key: %s to the object store",
+            object_key,
+        )
         presigned_post_response = s3_client.generate_presigned_post(
             Bucket=object_storage_config.bucket_name.get_secret_value(),
             Key=object_key,

object_storage_api/stores/image.py

@@ -29,7 +29,7 @@ def upload(self, image_id: str, image_metadata: ImagePostMetadataSchema, upload_
         """
         object_key = f"images/{image_metadata.entity_id}/{image_id}"
 
-        logger.info("Uploading image file to the object storage")
+        logger.info("Uploading image file with object key: %s to the object store", object_key)
         s3_client.upload_fileobj(
             upload_file.file,
             Bucket=object_storage_config.bucket_name.get_secret_value(),
@@ -46,7 +46,7 @@ def create_presigned_get(self, image: ImageOut) -> str:
         :param image: `ImageOut` model of the image.
         :return: Presigned url to get the image.
         """
-        logger.info("Generating presigned url to get image from object storage")
+        logger.info("Generating presigned url to get image with object key: %s from the object store", image.object_key)
         response = s3_client.generate_presigned_url(
             "get_object",
             Params={
@@ -58,3 +58,16 @@ def create_presigned_get(self, image: ImageOut) -> str:
         )
 
         return response
+
+    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 with object key: %s from the object store", object_key)
+        s3_client.delete_object(
+            Bucket=object_storage_config.bucket_name.get_secret_value(),
+            Key=object_key,
+        )

test/e2e/test_image.py

@@ -165,15 +165,15 @@ def post_test_images(self) -> list[dict]:
         """
         Posts three images. The first two images have the same entity ID, the last image has a different one.
 
-        :return: List of dictionaries containing the expected item data returned from a get endpoint in
+        :return: List of dictionaries containing the expected image data returned from a get endpoint in
                  the form of an `ImageMetadataSchema`.
         """
         entity_id_a, entity_id_b = (str(ObjectId()) for _ in range(2))
 
-        # First item
+        # First image
         image_a_id = self.post_image({**IMAGE_POST_METADATA_DATA_ALL_VALUES, "entity_id": entity_id_a}, "image.jpg")
 
-        # Second item
+        # Second image
         image_b_id = self.post_image(
             {
                 **IMAGE_POST_METADATA_DATA_ALL_VALUES,
@@ -182,7 +182,7 @@ def post_test_images(self) -> list[dict]:
             "image.jpg",
         )
 
-        # Third item
+        # Third image
         image_c_id = self.post_image(
             {
                 **IMAGE_POST_METADATA_DATA_ALL_VALUES,
@@ -305,3 +305,59 @@ def test_list_with_invalid_primary_filter(self):
             "Input should be a valid boolean, unable to interpret input",
             self._get_response_image.json()["detail"][0]["msg"],
         )
+
+
+class DeleteDSL(ListDSL):
+    """Base class for delete tests."""
+
+    _delete_response_image: Response
+
+    def delete_image(self, image_id: str) -> None:
+        """
+        Deletes an image with the given ID.
+
+        :param image_id: ID of the image to be deleted.
+        """
+
+        self._delete_response_image = self.test_client.delete(f"/images/{image_id}")
+
+    def check_delete_image_success(self) -> None:
+        """Checks that a prior call to `delete_image` gave a successful response with the expected code."""
+
+        assert self._delete_response_image.status_code == 204
+
+    def check_delete_image_failed_with_detail(self) -> None:
+        """
+        Checks that a prior call to `delete_image` gave a failed response with the expected code and
+        error message.
+        """
+
+        assert self._delete_response_image.status_code == 404
+        assert self._delete_response_image.json()["detail"] == "Image not found"
+
+
+class TestDelete(DeleteDSL):
+    """Tests for deleting an image."""
+
+    def test_delete(self):
+        """Test deleting an image."""
+
+        image_id = self.post_image(IMAGE_POST_METADATA_DATA_REQUIRED_VALUES_ONLY, "image.jpg")
+
+        self.delete_image(image_id)
+        self.check_delete_image_success()
+
+        self.get_image(image_id)
+        self.check_get_image_failed()
+
+    def test_delete_with_non_existent_id(self):
+        """Test deleting a non-existent image."""
+
+        self.delete_image(str(ObjectId()))
+        self.check_delete_image_failed_with_detail()
+
+    def test_delete_with_invalid_id(self):
+        """Test deleting an image with an invalid ID."""
+
+        self.delete_image("invalid_id")
+        self.check_delete_image_failed_with_detail()

test/unit/repositories/conftest.py

@@ -10,7 +10,7 @@
 from bson import ObjectId
 from pymongo.collection import Collection
 from pymongo.database import Database
-from pymongo.results import InsertOneResult
+from pymongo.results import DeleteResult, InsertOneResult
 
 
 @pytest.fixture(name="database_mock")
@@ -77,3 +77,17 @@ def mock_find(collection_mock: Mock, documents: List[dict]) -> None:
         cursor_mock = MagicMock(Cursor)
         cursor_mock.__iter__.return_value = iter(documents)
         collection_mock.find.return_value = cursor_mock
+
+    @staticmethod
+    def mock_delete_one(collection_mock: Mock, deleted_count: int) -> None:
+        """
+        Mock the `delete_one` method of the MongoDB database collection mock to return a `DeleteResult` object. The
+        passed `deleted_count` value is returned as the `deleted_count` attribute of the `DeleteResult` object, enabling
+        for the code that relies on the `deleted_count` value to work.
+
+        :param collection_mock: Mocked MongoDB database collection instance.
+        :param deleted_count: The value to be assigned to the `deleted_count` attribute of the `DeleteResult` object
+        """
+        delete_result_mock = Mock(DeleteResult)
+        delete_result_mock.deleted_count = deleted_count
+        collection_mock.delete_one.return_value = delete_result_mock

test/unit/repositories/test_image.py

@@ -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).
         """
 
@@ -277,4 +277,96 @@ def test_list_with_primary_and_entity_id(self):
         self.check_list_success()
 
 
+class DeleteDSL(ImageRepoDSL):
+    """Base class for `delete` tests."""
+
+    _delete_image_id: str
+    _delete_exception: pytest.ExceptionInfo
+
+    def mock_delete(self, deleted_count: int) -> None:
+        """
+        Mocks database methods appropriately to test the `delete` repo method.
+
+        :param deleted_count: Number of documents deleted successfully.
+        """
+        RepositoryTestHelpers.mock_delete_one(self.images_collection, deleted_count)
+
+    def call_delete(self, image_id: str) -> None:
+        """
+        Calls the `ImageRepo` `delete` method.
+
+        :param image_id: ID of the image to be deleted.
+        """
+
+        self._delete_image_id = image_id
+        self.image_repository.delete(image_id, session=self.mock_session)
+
+    def call_delete_expecting_error(self, image_id: str, error_type: type[BaseException]) -> None:
+        """
+        Calls the `ImageRepo` `delete` method while expecting an error to be raised.
+
+        :param image_id: ID of the image to be deleted.
+        :param error_type: Expected exception to be raised.
+        """
+
+        self._delete_image_id = image_id
+        with pytest.raises(error_type) as exc:
+            self.image_repository.delete(image_id, session=self.mock_session)
+        self._delete_exception = exc
+
+    def check_delete_success(self) -> None:
+        """Checks that a prior call to `call_delete` worked as expected."""
+
+        self.images_collection.delete_one.assert_called_once_with(
+            filter={"_id": ObjectId(self._delete_image_id)}, session=self.mock_session
+        )
+
+    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 `find_one_and_delete` method is expected to be called or not.
+        """
+
+        if not assert_delete:
+            self.images_collection.delete_one.assert_not_called()
+        else:
+            self.images_collection.delete_one.assert_called_once_with(
+                filter={"_id": ObjectId(self._delete_image_id)},
+                session=self.mock_session,
+            )
+
+        assert str(self._delete_exception.value) == message
+
+
+class TestDelete(DeleteDSL):
+    """Tests for deleting an image."""
+
+    def test_delete(self):
+        """Test deleting an image."""
+
+        self.mock_delete(1)
+        self.call_delete(str(ObjectId()))
+        self.check_delete_success()
+
+    def test_delete_non_existent_id(self):
+        """Test deleting an image with a non-existent ID."""
+
+        image_id = str(ObjectId())
+
+        self.mock_delete(0)
+        self.call_delete_expecting_error(image_id, MissingRecordError)
+        self.check_delete_failed_with_exception(f"No image found with ID: {image_id}", assert_delete=True)
+
+    def test_delete_invalid_id(self):
+        """Test deleting an image with an invalid ID."""
+
+        image_id = "invalid-id"
+
+        self.call_delete_expecting_error(image_id, InvalidObjectIdError)
+        self.check_delete_failed_with_exception(f"Invalid ObjectId value '{image_id}'")
+
+
 # pylint: enable=duplicate-code

test/unit/services/test_image.py

@@ -247,3 +247,45 @@ def test_list(self):
         self.mock_list()
         self.call_list(entity_id=str(ObjectId()), primary=False)
         self.check_list_success()
+
+
+class DeleteDSL(ImageServiceDSL):
+    """Base class for `delete` tests."""
+
+    _delete_image_id: str
+    _expected_image_out: ImageOut
+    _delete_image_id: str
+    _delete_image_object_key: str
+
+    def mock_delete(self, image_in_data: dict) -> None:
+        """
+        Mocks repo methods appropriately to test the `delete` service method.
+
+        :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).
+        """
+        self._expected_image_out = ImageOut(**ImageIn(**image_in_data).model_dump())
+        self.mock_image_repository.get.return_value = self._expected_image_out
+        self._delete_image_id = self._expected_image_out.id
+        self._delete_image_object_key = self._expected_image_out.object_key
+
+    def call_delete(self) -> None:
+        """Calls the `ImageService` `delete` method."""
+
+        self.image_service.delete(self._delete_image_id)
+
+    def check_delete_success(self) -> None:
+        """Checks that a prior call to `call_delete` worked as expected."""
+
+        self.mock_image_store.delete.assert_called_once_with(self._delete_image_object_key)
+        self.mock_image_repository.delete.assert_called_once_with(self._delete_image_id)
+
+
+class TestDelete(DeleteDSL):
+    """Tests for deleting an image."""
+
+    def test_delete(self):
+        """Test for deleting an image."""
+        self.mock_delete(IMAGE_IN_DATA_ALL_VALUES)
+        self.call_delete()
+        self.check_delete_success()