From 094fb83247bd77871fc5064639bf66fb085a4a03 Mon Sep 17 00:00:00 2001 From: Anikesh Suresh Date: Fri, 6 Dec 2024 11:32:30 +0000 Subject: [PATCH] Address PR Review Comments #38 --- object_storage_api/routers/image.py | 2 +- object_storage_api/services/image.py | 1 + object_storage_api/stores/attachment.py | 5 ++++- object_storage_api/stores/image.py | 6 +++--- test/unit/services/test_image.py | 24 ++++++++++++++++++------ test/unit/stores/test_image.py | 14 ++++++++------ 6 files changed, 35 insertions(+), 17 deletions(-) diff --git a/object_storage_api/routers/image.py b/object_storage_api/routers/image.py index 92b969f..e5c0e05 100644 --- a/object_storage_api/routers/image.py +++ b/object_storage_api/routers/image.py @@ -83,7 +83,7 @@ def get_image( @router.delete( path="/{image_id}", summary="Delete an image by ID", - response_description="Image deleted Sucessfully", + response_description="Image deleted sucessfully", status_code=status.HTTP_204_NO_CONTENT, ) def delete_image( diff --git a/object_storage_api/services/image.py b/object_storage_api/services/image.py index d4341f8..f9906d9 100644 --- a/object_storage_api/services/image.py +++ b/object_storage_api/services/image.py @@ -104,5 +104,6 @@ def delete(self, image_id: str) -> None: :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 e3d8a1f..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={ @@ -66,7 +66,7 @@ def delete(self, object_key: str) -> None: :param object_key: Key of the image to delete. """ - logger.info("Deleting image file from the object storage") + 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/unit/services/test_image.py b/test/unit/services/test_image.py index 0886631..2d40342 100644 --- a/test/unit/services/test_image.py +++ b/test/unit/services/test_image.py @@ -253,21 +253,32 @@ 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 call_delete(self, image_id: str) -> None: + def mock_delete(self, image_in_data: dict) -> None: """ - Calls the `ImageService` `delete` method. + Mocks repo methods appropriately to test the `delete` service method. - :param image_id: ID of the image to be deleted. + :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._delete_image_id = image_id - self.image_service.delete(image_id) + 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_repository.delete.assert_called_once_with(self._delete_image_id) + self.mock_image_store.delete.assert_called_once_with(self._delete_image_object_key) class TestDelete(DeleteDSL): @@ -275,5 +286,6 @@ class TestDelete(DeleteDSL): def test_delete(self): """Test for deleting an image.""" - self.call_delete("test_image_id") + 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 f8fcb4f..936ee73 100644 --- a/test/unit/stores/test_image.py +++ b/test/unit/stores/test_image.py @@ -88,27 +88,29 @@ def test_upload(self): class DeleteDSL(ImageStoreDSL): """Base class for `delete` tests.""" - def call_delete(self) -> None: - """Calls the `ImageStore` `delete` method.""" + _delete_object_key: str - self.image_store.delete("object_key") + 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="object_key", + Key=self._delete_object_key, ) class TestDelete(DeleteDSL): """Tests for deleting an image from object storage.""" - def test_create_presigned_post(self): + def test_delete(self): """Test for deleting an image from object storage.""" - self.call_delete() + self.call_delete("object-key") self.check_delete_success()