Merge pull request #94 from ral-facilities/add-download-url-to-endpoi…

…nt-for-getting-a-single-image-#93

Add download url to endpoint for getting a single image #93

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")
+    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")

object_storage_api/services/image.py

@@ -90,14 +90,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 url 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 a presigned get url.
+        :return: An image's metadata with its presigned get urls.
         """
         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)
+        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]:
         """

object_storage_api/stores/image.py

@@ -39,25 +39,38 @@ 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.
 
         :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)
-        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,
+        }
+
+        view_url = s3_client.generate_presigned_url(**parameters)
+
+        download_url = s3_client.generate_presigned_url(
+            **{
+                **parameters,
+                "Params": {
+                    **parameters["Params"],
+                    "ResponseContentDisposition": f'attachment; filename="{image.file_name}"',
+                },
+            }
         )
 
-        return response
+        return view_url, download_url
 
     def delete(self, object_key: str) -> None:
         """

test/mock_data.py

@@ -185,5 +185,6 @@
 
 IMAGE_GET_DATA_ALL_VALUES = {
     **IMAGE_GET_METADATA_DATA_ALL_VALUES,
-    "url": ANY,
+    "view_url": ANY,
+    "download_url": ANY,
 }

test/unit/repositories/test_image.py

@@ -299,10 +299,7 @@ def set_update_data(self, new_image_in_data: dict):
         """
         self._image_in = ImageIn(**new_image_in_data)
 
-    def mock_update(
-        self,
-        new_image_in_data: dict,
-    ) -> None:
+    def mock_update(self, new_image_in_data: dict) -> None:
         """
         Mocks database methods appropriately to test the `update` repo method.
 

test/unit/services/test_image.py

@@ -198,9 +198,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(),
+            view_url="https://fakepresignedurl.co.uk/inline",
+            download_url="https://fakepresignedurl.co.uk/attachment",
         )
 
     def call_get(self, image_id: str) -> None:

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
@@ -122,8 +122,10 @@ class CreatePresignedURLDSL(ImageStoreDSL):
     """Base class for `create` tests."""
 
     _image_out: ImageOut
-    _expected_presigned_url: str
-    _obtained_presigned_url: str
+    _expected_presigned_view_url: str
+    _obtained_presigned_view_url: str
+    _expected_presigned_download_url: str
+    _obtained_presigned_download_url: str
 
     def mock_create_presigned_get(self, image_in_data: dict) -> None:
         """
@@ -135,38 +137,59 @@ 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_view_url = "example_presigned_view_url"
+        self._expected_presigned_download_url = "example_presigned_download_url"
+        self.mock_s3_client.generate_presigned_url.side_effect = [
+            self._expected_presigned_view_url,
+            self._expected_presigned_download_url,
+        ]
 
     def call_create_presigned_get(self) -> None:
         """
         Calls the `ImageStore` `create_presigned_get` method with the appropriate data from a prior call to
             `mock_create_presigned_get`.
         """
 
-        self._obtained_presigned_url = self.image_store.create_presigned_get(self._image_out)
+        (self._obtained_presigned_view_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={
+        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,
-        )
-
-        assert self._obtained_presigned_url == self._expected_presigned_url
+            "ExpiresIn": object_storage_config.presigned_url_expiry_seconds,
+        }
+
+        expected_calls = [
+            call.generate_presigned_url(**parameters),
+            call.generate_presigned_url(
+                **{
+                    **parameters,
+                    "Params": {
+                        **parameters["Params"],
+                        "ResponseContentDisposition": f'attachment; filename="{self._image_out.file_name}"',
+                    },
+                }
+            ),
+        ]
+        self.mock_s3_client.assert_has_calls(expected_calls)
+
+        assert self._obtained_presigned_view_url == self._expected_presigned_view_url
+        assert self._obtained_presigned_download_url == self._expected_presigned_download_url
 
 
 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()