diff --git a/object_storage_api/repositories/image.py b/object_storage_api/repositories/image.py index ec0a7cf..be2081a 100644 --- a/object_storage_api/repositories/image.py +++ b/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") diff --git a/object_storage_api/routers/image.py b/object_storage_api/routers/image.py index 73dcca9..4ed6e6e 100644 --- a/object_storage_api/routers/image.py +++ b/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) diff --git a/object_storage_api/services/image.py b/object_storage_api/services/image.py index 1157510..f9906d9 100644 --- a/object_storage_api/services/image.py +++ b/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) diff --git a/object_storage_api/stores/attachment.py b/object_storage_api/stores/attachment.py index c88f9d3..136b7a8 100644 --- a/object_storage_api/stores/attachment.py +++ b/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, diff --git a/object_storage_api/stores/image.py b/object_storage_api/stores/image.py index 26083c9..31ea881 100644 --- a/object_storage_api/stores/image.py +++ b/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, + ) diff --git a/test/e2e/test_image.py b/test/e2e/test_image.py index b1df7ed..274ca95 100644 --- a/test/e2e/test_image.py +++ b/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() diff --git a/test/unit/repositories/conftest.py b/test/unit/repositories/conftest.py index d8a2874..e45570b 100644 --- a/test/unit/repositories/conftest.py +++ b/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 diff --git a/test/unit/repositories/test_image.py b/test/unit/repositories/test_image.py index c832a22..dfe0d41 100644 --- a/test/unit/repositories/test_image.py +++ b/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 diff --git a/test/unit/services/test_image.py b/test/unit/services/test_image.py index 07bb88f..d39649a 100644 --- a/test/unit/services/test_image.py +++ b/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() diff --git a/test/unit/stores/test_image.py b/test/unit/stores/test_image.py index 55282b9..936ee73 100644 --- a/test/unit/stores/test_image.py +++ b/test/unit/stores/test_image.py @@ -85,6 +85,35 @@ def test_upload(self): self.check_upload_success() +class DeleteDSL(ImageStoreDSL): + """Base class for `delete` tests.""" + + _delete_object_key: str + + def call_delete(self, object_key: str) -> None: + """Calls the `ImageStore` `delete` method.""" + self._delete_object_key = object_key + self.image_store.delete(object_key) + + def check_delete_success(self) -> None: + """Checks that a prior call to `call_delete` worked as expected.""" + + self.mock_s3_client.delete_object.assert_called_once_with( + Bucket=object_storage_config.bucket_name.get_secret_value(), + Key=self._delete_object_key, + ) + + +class TestDelete(DeleteDSL): + """Tests for deleting an image from object storage.""" + + def test_delete(self): + """Test for deleting an image from object storage.""" + + self.call_delete("object-key") + self.check_delete_success() + + class CreatePresignedURLDSL(ImageStoreDSL): """Base class for `create` tests."""