From bbeb0292b17c06caa43b051365c49f8c9e97c8ac Mon Sep 17 00:00:00 2001 From: Anikesh Suresh Date: Thu, 16 Jan 2025 10:24:52 +0000 Subject: [PATCH 1/3] added download url, to implementation and tests TODO: check docstrings, and clean up code #93 --- object_storage_api/schemas/image.py | 3 +- object_storage_api/services/image.py | 4 +- object_storage_api/stores/image.py | 17 +++++++-- test/mock_data.py | 3 +- test/unit/services/test_image.py | 9 ++++- test/unit/stores/test_image.py | 55 +++++++++++++++++++--------- 6 files changed, 65 insertions(+), 26 deletions(-) diff --git a/object_storage_api/schemas/image.py b/object_storage_api/schemas/image.py index 5f25e06..2650821 100644 --- a/object_storage_api/schemas/image.py +++ b/object_storage_api/schemas/image.py @@ -38,4 +38,5 @@ class ImageMetadataSchema(CreatedModifiedSchemaMixin, ImagePostMetadataSchema): class ImageSchema(ImageMetadataSchema): """Schema model for an image get request response.""" - url: HttpUrl = Field(description="Presigned get URL to get the image file") + inline_url: HttpUrl = Field(description="Presigned get URL to view the image file") + download_url: HttpUrl = Field(description="Presigned get URL to download the image file") diff --git a/object_storage_api/services/image.py b/object_storage_api/services/image.py index 4c92914..7f70711 100644 --- a/object_storage_api/services/image.py +++ b/object_storage_api/services/image.py @@ -88,8 +88,8 @@ def get(self, image_id: str) -> ImageSchema: :return: An image's metadata with a presigned get url. """ image = self._image_repository.get(image_id=image_id) - presigned_url = self._image_store.create_presigned_get(image) - return ImageSchema(**image.model_dump(), url=presigned_url) + (inline_url, download_url) = self._image_store.create_presigned_get(image) + return ImageSchema(**image.model_dump(), inline_url=inline_url, download_url=download_url) def list(self, entity_id: Optional[str] = None, primary: Optional[bool] = None) -> list[ImageMetadataSchema]: """ diff --git a/object_storage_api/stores/image.py b/object_storage_api/stores/image.py index 31ea881..b931d8a 100644 --- a/object_storage_api/stores/image.py +++ b/object_storage_api/stores/image.py @@ -39,7 +39,7 @@ def upload(self, image_id: str, image_metadata: ImagePostMetadataSchema, upload_ return object_key - def create_presigned_get(self, image: ImageOut) -> str: + def create_presigned_get(self, image: ImageOut) -> tuple[str, str]: """ Generate a presigned URL to share an S3 object. @@ -47,7 +47,8 @@ def create_presigned_get(self, image: ImageOut) -> str: :return: Presigned url to get the image. """ 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( + + inline_response = s3_client.generate_presigned_url( "get_object", Params={ "Bucket": object_storage_config.bucket_name.get_secret_value(), @@ -57,7 +58,17 @@ def create_presigned_get(self, image: ImageOut) -> str: ExpiresIn=object_storage_config.presigned_url_expiry_seconds, ) - return response + attachment_response = s3_client.generate_presigned_url( + "get_object", + Params={ + "Bucket": object_storage_config.bucket_name.get_secret_value(), + "Key": image.object_key, + "ResponseContentDisposition": f'attachment; filename="{image.file_name}"', + }, + ExpiresIn=object_storage_config.presigned_url_expiry_seconds, + ) + + return (inline_response, attachment_response) def delete(self, object_key: str) -> None: """ diff --git a/test/mock_data.py b/test/mock_data.py index 60d4e01..a7e4313 100644 --- a/test/mock_data.py +++ b/test/mock_data.py @@ -170,5 +170,6 @@ IMAGE_GET_DATA_ALL_VALUES = { **IMAGE_GET_METADATA_DATA_ALL_VALUES, - "url": ANY, + "inline_url": ANY, + "download_url": ANY, } diff --git a/test/unit/services/test_image.py b/test/unit/services/test_image.py index 39fd332..a0017d0 100644 --- a/test/unit/services/test_image.py +++ b/test/unit/services/test_image.py @@ -185,9 +185,14 @@ def mock_get(self) -> None: self._expected_image_out = ImageOut(**ImageIn(**IMAGE_IN_DATA_ALL_VALUES).model_dump()) self.mock_image_repository.get.return_value = self._expected_image_out - self.mock_image_store.create_presigned_get.return_value = "https://fakepresignedurl.co.uk" + self.mock_image_store.create_presigned_get.return_value = ( + "https://fakepresignedurl.co.uk/inline", + "https://fakepresignedurl.co.uk/attachment", + ) self._expected_image = ImageSchema( - **self._expected_image_out.model_dump(), url="https://fakepresignedurl.co.uk" + **self._expected_image_out.model_dump(), + inline_url="https://fakepresignedurl.co.uk/inline", + download_url="https://fakepresignedurl.co.uk/attachment", ) def call_get(self, image_id: str) -> None: diff --git a/test/unit/stores/test_image.py b/test/unit/stores/test_image.py index 936ee73..42fa8af 100644 --- a/test/unit/stores/test_image.py +++ b/test/unit/stores/test_image.py @@ -3,7 +3,7 @@ """ from test.mock_data import IMAGE_IN_DATA_ALL_VALUES, IMAGE_POST_METADATA_DATA_ALL_VALUES -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch import pytest from bson import ObjectId @@ -118,8 +118,10 @@ class CreatePresignedURLDSL(ImageStoreDSL): """Base class for `create` tests.""" _image_out: ImageOut - _expected_presigned_url: str - _obtained_presigned_url: str + _expected_presigned_inline_url: str + _obtained_presigned_inline_url: str + _expected_presigned_download_url: str + _obtained_presigned_download_url: str def mock_create_presigned_get(self, image_in_data: dict) -> None: """ @@ -131,8 +133,12 @@ def mock_create_presigned_get(self, image_in_data: dict) -> None: self._image_out = ImageOut(**ImageIn(**image_in_data).model_dump()) # Mock presigned url generation - self._expected_presigned_url = "example_presigned_url" - self.mock_s3_client.generate_presigned_url.return_value = self._expected_presigned_url + self._expected_presigned_inline_url = "example_presigned_inline_url" + self._expected_presigned_download_url = "example_presigned_downloadurl" + self.mock_s3_client.generate_presigned_url.side_effect = [ + self._expected_presigned_inline_url, + self._expected_presigned_download_url, + ] def call_create_presigned_get(self) -> None: """ @@ -140,22 +146,37 @@ def call_create_presigned_get(self) -> None: `mock_create_presigned_get`. """ - self._obtained_presigned_url = self.image_store.create_presigned_get(self._image_out) + (self._obtained_presigned_inline_url, self._obtained_presigned_download_url) = ( + self.image_store.create_presigned_get(self._image_out) + ) def check_create_presigned_get_success(self) -> None: """Checks that a prior call to `call_create_presigned_get` worked as expected.""" - self.mock_s3_client.generate_presigned_url.assert_called_once_with( - "get_object", - Params={ - "Bucket": object_storage_config.bucket_name.get_secret_value(), - "Key": self._image_out.object_key, - "ResponseContentDisposition": f'inline; filename="{self._image_out.file_name}"', - }, - ExpiresIn=object_storage_config.presigned_url_expiry_seconds, - ) - - assert self._obtained_presigned_url == self._expected_presigned_url + expected_calls = [ + call.generate_presigned_url( + "get_object", + Params={ + "Bucket": object_storage_config.bucket_name.get_secret_value(), + "Key": self._image_out.object_key, + "ResponseContentDisposition": f'inline; filename="{self._image_out.file_name}"', + }, + ExpiresIn=object_storage_config.presigned_url_expiry_seconds, + ), + call.generate_presigned_url( + "get_object", + Params={ + "Bucket": object_storage_config.bucket_name.get_secret_value(), + "Key": self._image_out.object_key, + "ResponseContentDisposition": f'attachment; filename="{self._image_out.file_name}"', + }, + ExpiresIn=object_storage_config.presigned_url_expiry_seconds, + ), + ] + self.mock_s3_client.assert_has_calls(expected_calls) + + assert self._obtained_presigned_inline_url == self._expected_presigned_inline_url + assert self._obtained_presigned_download_url == self._expected_presigned_download_url class TestCreatePresignedURL(CreatePresignedURLDSL): From 5da061b087beef207c0bd928c56122943285bff2 Mon Sep 17 00:00:00 2001 From: Anikesh Suresh Date: Thu, 16 Jan 2025 11:23:08 +0000 Subject: [PATCH 2/3] Consolidate code and update docstrings #93 --- object_storage_api/services/image.py | 4 +-- object_storage_api/stores/image.py | 28 ++++++++++---------- test/unit/stores/test_image.py | 38 +++++++++++++++------------- 3 files changed, 37 insertions(+), 33 deletions(-) diff --git a/object_storage_api/services/image.py b/object_storage_api/services/image.py index 7f70711..647b71d 100644 --- a/object_storage_api/services/image.py +++ b/object_storage_api/services/image.py @@ -82,10 +82,10 @@ def create(self, image_metadata: ImagePostMetadataSchema, upload_file: UploadFil def get(self, image_id: str) -> ImageSchema: """ - Retrieve an image's metadata with its presigned get url by its ID. + Retrieve an image's metadata with its presigned get download and inline urls by its ID. :param image_id: ID of the image to retrieve. - :return: An image's metadata with a presigned get url. + :return: An image's metadata with its presigned get urls. """ image = self._image_repository.get(image_id=image_id) (inline_url, download_url) = self._image_store.create_presigned_get(image) diff --git a/object_storage_api/stores/image.py b/object_storage_api/stores/image.py index b931d8a..4a316c2 100644 --- a/object_storage_api/stores/image.py +++ b/object_storage_api/stores/image.py @@ -44,28 +44,30 @@ def create_presigned_get(self, image: ImageOut) -> tuple[str, str]: Generate a presigned URL to share an S3 object. :param image: `ImageOut` model of the image. - :return: Presigned url to get the image. + :return: Presigned urls to view and download the image. """ logger.info("Generating presigned url to get image with object key: %s from the object store", image.object_key) - inline_response = s3_client.generate_presigned_url( - "get_object", - Params={ + parameters = { + "ClientMethod": "get_object", + "Params": { "Bucket": object_storage_config.bucket_name.get_secret_value(), "Key": image.object_key, "ResponseContentDisposition": f'inline; filename="{image.file_name}"', }, - ExpiresIn=object_storage_config.presigned_url_expiry_seconds, - ) + "ExpiresIn": object_storage_config.presigned_url_expiry_seconds, + } + + inline_response = s3_client.generate_presigned_url(**parameters) attachment_response = s3_client.generate_presigned_url( - "get_object", - Params={ - "Bucket": object_storage_config.bucket_name.get_secret_value(), - "Key": image.object_key, - "ResponseContentDisposition": f'attachment; filename="{image.file_name}"', - }, - ExpiresIn=object_storage_config.presigned_url_expiry_seconds, + **{ + **parameters, + "Params": { + **parameters["Params"], + "ResponseContentDisposition": f'attachment; filename="{image.file_name}"', + }, + } ) return (inline_response, attachment_response) diff --git a/test/unit/stores/test_image.py b/test/unit/stores/test_image.py index 42fa8af..22a9476 100644 --- a/test/unit/stores/test_image.py +++ b/test/unit/stores/test_image.py @@ -153,24 +153,26 @@ def call_create_presigned_get(self) -> None: def check_create_presigned_get_success(self) -> None: """Checks that a prior call to `call_create_presigned_get` worked as expected.""" + parameters = { + "ClientMethod": "get_object", + "Params": { + "Bucket": object_storage_config.bucket_name.get_secret_value(), + "Key": self._image_out.object_key, + "ResponseContentDisposition": f'inline; filename="{self._image_out.file_name}"', + }, + "ExpiresIn": object_storage_config.presigned_url_expiry_seconds, + } + expected_calls = [ + call.generate_presigned_url(**parameters), call.generate_presigned_url( - "get_object", - Params={ - "Bucket": object_storage_config.bucket_name.get_secret_value(), - "Key": self._image_out.object_key, - "ResponseContentDisposition": f'inline; filename="{self._image_out.file_name}"', - }, - ExpiresIn=object_storage_config.presigned_url_expiry_seconds, - ), - call.generate_presigned_url( - "get_object", - Params={ - "Bucket": object_storage_config.bucket_name.get_secret_value(), - "Key": self._image_out.object_key, - "ResponseContentDisposition": f'attachment; filename="{self._image_out.file_name}"', - }, - ExpiresIn=object_storage_config.presigned_url_expiry_seconds, + **{ + **parameters, + "Params": { + **parameters["Params"], + "ResponseContentDisposition": f'attachment; filename="{self._image_out.file_name}"', + }, + } ), ] self.mock_s3_client.assert_has_calls(expected_calls) @@ -180,10 +182,10 @@ def check_create_presigned_get_success(self) -> None: class TestCreatePresignedURL(CreatePresignedURLDSL): - """Tests for creating a presigned url for an image.""" + """Tests for creating presigned get urls for an image.""" def test_create_presigned_get(self): - """Test creating a presigned url for an image.""" + """Test creating presigned get urls for an image.""" self.mock_create_presigned_get(IMAGE_IN_DATA_ALL_VALUES) self.call_create_presigned_get() From f87998efd5742ebe8354dd777ab3cc4774cdb857 Mon Sep 17 00:00:00 2001 From: Anikesh Suresh Date: Thu, 16 Jan 2025 11:54:08 +0000 Subject: [PATCH 3/3] Rename inline to view in var names #93 --- object_storage_api/schemas/image.py | 2 +- object_storage_api/services/image.py | 6 +++--- object_storage_api/stores/image.py | 4 ++-- test/mock_data.py | 2 +- test/unit/services/test_image.py | 2 +- test/unit/stores/test_image.py | 12 ++++++------ 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/object_storage_api/schemas/image.py b/object_storage_api/schemas/image.py index 2650821..7b5c4ca 100644 --- a/object_storage_api/schemas/image.py +++ b/object_storage_api/schemas/image.py @@ -38,5 +38,5 @@ class ImageMetadataSchema(CreatedModifiedSchemaMixin, ImagePostMetadataSchema): class ImageSchema(ImageMetadataSchema): """Schema model for an image get request response.""" - inline_url: HttpUrl = Field(description="Presigned get URL to view the image file") + view_url: HttpUrl = Field(description="Presigned get URL to view the image file") download_url: HttpUrl = Field(description="Presigned get URL to download the image file") diff --git a/object_storage_api/services/image.py b/object_storage_api/services/image.py index 647b71d..da95074 100644 --- a/object_storage_api/services/image.py +++ b/object_storage_api/services/image.py @@ -82,14 +82,14 @@ def create(self, image_metadata: ImagePostMetadataSchema, upload_file: UploadFil def get(self, image_id: str) -> ImageSchema: """ - Retrieve an image's metadata with its presigned get download and inline urls by its ID. + Retrieve an image's metadata with its presigned get download and view urls by its ID. :param image_id: ID of the image to retrieve. :return: An image's metadata with its presigned get urls. """ image = self._image_repository.get(image_id=image_id) - (inline_url, download_url) = self._image_store.create_presigned_get(image) - return ImageSchema(**image.model_dump(), inline_url=inline_url, download_url=download_url) + (view_url, download_url) = self._image_store.create_presigned_get(image) + return ImageSchema(**image.model_dump(), view_url=view_url, download_url=download_url) def list(self, entity_id: Optional[str] = None, primary: Optional[bool] = None) -> list[ImageMetadataSchema]: """ diff --git a/object_storage_api/stores/image.py b/object_storage_api/stores/image.py index 4a316c2..9821844 100644 --- a/object_storage_api/stores/image.py +++ b/object_storage_api/stores/image.py @@ -58,7 +58,7 @@ def create_presigned_get(self, image: ImageOut) -> tuple[str, str]: "ExpiresIn": object_storage_config.presigned_url_expiry_seconds, } - inline_response = s3_client.generate_presigned_url(**parameters) + view_response = s3_client.generate_presigned_url(**parameters) attachment_response = s3_client.generate_presigned_url( **{ @@ -70,7 +70,7 @@ def create_presigned_get(self, image: ImageOut) -> tuple[str, str]: } ) - return (inline_response, attachment_response) + return (view_response, attachment_response) def delete(self, object_key: str) -> None: """ diff --git a/test/mock_data.py b/test/mock_data.py index a7e4313..67030df 100644 --- a/test/mock_data.py +++ b/test/mock_data.py @@ -170,6 +170,6 @@ IMAGE_GET_DATA_ALL_VALUES = { **IMAGE_GET_METADATA_DATA_ALL_VALUES, - "inline_url": ANY, + "view_url": ANY, "download_url": ANY, } diff --git a/test/unit/services/test_image.py b/test/unit/services/test_image.py index a0017d0..b0d5abd 100644 --- a/test/unit/services/test_image.py +++ b/test/unit/services/test_image.py @@ -191,7 +191,7 @@ def mock_get(self) -> None: ) self._expected_image = ImageSchema( **self._expected_image_out.model_dump(), - inline_url="https://fakepresignedurl.co.uk/inline", + view_url="https://fakepresignedurl.co.uk/inline", download_url="https://fakepresignedurl.co.uk/attachment", ) diff --git a/test/unit/stores/test_image.py b/test/unit/stores/test_image.py index 22a9476..cab1b82 100644 --- a/test/unit/stores/test_image.py +++ b/test/unit/stores/test_image.py @@ -118,8 +118,8 @@ class CreatePresignedURLDSL(ImageStoreDSL): """Base class for `create` tests.""" _image_out: ImageOut - _expected_presigned_inline_url: str - _obtained_presigned_inline_url: str + _expected_presigned_view_url: str + _obtained_presigned_view_url: str _expected_presigned_download_url: str _obtained_presigned_download_url: str @@ -133,10 +133,10 @@ def mock_create_presigned_get(self, image_in_data: dict) -> None: self._image_out = ImageOut(**ImageIn(**image_in_data).model_dump()) # Mock presigned url generation - self._expected_presigned_inline_url = "example_presigned_inline_url" + self._expected_presigned_view_url = "example_presigned_view_url" self._expected_presigned_download_url = "example_presigned_downloadurl" self.mock_s3_client.generate_presigned_url.side_effect = [ - self._expected_presigned_inline_url, + self._expected_presigned_view_url, self._expected_presigned_download_url, ] @@ -146,7 +146,7 @@ def call_create_presigned_get(self) -> None: `mock_create_presigned_get`. """ - (self._obtained_presigned_inline_url, self._obtained_presigned_download_url) = ( + (self._obtained_presigned_view_url, self._obtained_presigned_download_url) = ( self.image_store.create_presigned_get(self._image_out) ) @@ -177,7 +177,7 @@ def check_create_presigned_get_success(self) -> None: ] self.mock_s3_client.assert_has_calls(expected_calls) - assert self._obtained_presigned_inline_url == self._expected_presigned_inline_url + assert self._obtained_presigned_view_url == self._expected_presigned_view_url assert self._obtained_presigned_download_url == self._expected_presigned_download_url