From 50ef621624ed3ddee5c7113b634c5cb6a6e09c13 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Mon, 25 Nov 2024 11:28:07 +0000 Subject: [PATCH 01/15] Add list functions for attachments in repositories/ and services/ --- object_storage_api/repositories/attachment.py | 23 +++++++++++++++++++ object_storage_api/services/attachment.py | 13 ++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/object_storage_api/repositories/attachment.py b/object_storage_api/repositories/attachment.py index e954530..200d299 100644 --- a/object_storage_api/repositories/attachment.py +++ b/object_storage_api/repositories/attachment.py @@ -56,3 +56,26 @@ def get(self, attachment_id: str, session: ClientSession = None) -> Optional[Att if attachment: return AttachmentOut(**attachment) return None + + def list(self, entity_id: Optional[str], session: ClientSession = None) -> list[AttachmentOut]: + """ + Retrieve attachments from a MongoDB database. + + :param session: PyMongo ClientSession to use for database operations. + :param entity_id: The ID of the entity to filter attachments by. + :return: List of attachments or an empty list if no attachments are retrieved. + """ + + query = {} + if entity_id is not None: + query["entity_id"] = CustomObjectId(entity_id) + + message = "Retrieving all attachments from the database" + if not query: + logger.info(message) + else: + logger.info("%s matching the provided filter(s)", message) + logger.debug("Provided filter(s): %s", query) + + attachments = self._attachments_collection.find(query, session=session) + return [AttachmentOut(**attachment) for attachment in attachments] diff --git a/object_storage_api/services/attachment.py b/object_storage_api/services/attachment.py index 261e360..6bebdf9 100644 --- a/object_storage_api/services/attachment.py +++ b/object_storage_api/services/attachment.py @@ -4,7 +4,7 @@ """ import logging -from typing import Annotated +from typing import Annotated, Optional from bson import ObjectId from fastapi import Depends @@ -62,3 +62,14 @@ def create(self, attachment: AttachmentPostSchema) -> AttachmentPostResponseSche attachment_out = self._attachment_repository.create(attachment_in) return AttachmentPostResponseSchema(**attachment_out.model_dump(), upload_info=upload_info) + + def list(self, entity_id: Optional[str] = None) -> list[AttachmentPostResponseSchema]: + """ + Retrieve a list of attachments based on the provided filters. + + :param entity_id: The ID of the entity to filter attachments by. + :return: List of attachments or an empty list if no attachments are retrieved. + """ + + attachments = self._attachment_repository.list(entity_id) + return [AttachmentPostResponseSchema(**attachment.model_dump()) for attachment in attachments] From b69e50df405bac0a5ef1da9bb2b20be5ec418f15 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Tue, 26 Nov 2024 18:55:42 +0000 Subject: [PATCH 02/15] Add get_attachments function in routers/ --- object_storage_api/routers/attachment.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/object_storage_api/routers/attachment.py b/object_storage_api/routers/attachment.py index 6d8bdb9..0e3506e 100644 --- a/object_storage_api/routers/attachment.py +++ b/object_storage_api/routers/attachment.py @@ -6,7 +6,7 @@ import logging from typing import Annotated -from fastapi import APIRouter, Depends, status +from fastapi import APIRouter, Depends, Query, status from object_storage_api.schemas.attachment import AttachmentPostResponseSchema, AttachmentPostSchema from object_storage_api.services.attachment import AttachmentService @@ -33,3 +33,20 @@ def create_attachment( logger.debug("Attachment data: %s", attachment) return attachment_service.create(attachment) + +@router.get( + path="", + summary="Get attachments", + response_description="List of attachments", +) +def get_attachments( + attachment_service: AttachmentServiceDep, + entity_id: Annotated[Optional[str], Query(description="Filter attachments by entity ID")] = None, +) -> list[AttachmentPostResponseSchema]: + # pylint: disable=missing-function-docstring + logger.info("Getting attachments") + + if entity_id is not None: + logger.debug("Entity ID filter: '%s'", entity_id) + + return attachment_service.list(entity_id) From d07fca035bab29193169baa140986c0d4829a978 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Wed, 27 Nov 2024 16:51:30 +0000 Subject: [PATCH 03/15] Add missing import in routers/attachment.py --- object_storage_api/routers/attachment.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/object_storage_api/routers/attachment.py b/object_storage_api/routers/attachment.py index 0e3506e..f8e722d 100644 --- a/object_storage_api/routers/attachment.py +++ b/object_storage_api/routers/attachment.py @@ -4,7 +4,7 @@ """ import logging -from typing import Annotated +from typing import Annotated, Optional from fastapi import APIRouter, Depends, Query, status From 6dea9d831522a0d2fb04aa280c9c662197be1bb7 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Thu, 28 Nov 2024 16:55:20 +0000 Subject: [PATCH 04/15] Fix failing get request by adding upload_info for each attachment - get request would fail previously as AttachmentPostResponseSchema was missing the upload_info argument - had to generate upload_info for each attachment --- object_storage_api/services/attachment.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/object_storage_api/services/attachment.py b/object_storage_api/services/attachment.py index 6bebdf9..594b3e8 100644 --- a/object_storage_api/services/attachment.py +++ b/object_storage_api/services/attachment.py @@ -72,4 +72,11 @@ def list(self, entity_id: Optional[str] = None) -> list[AttachmentPostResponseSc """ attachments = self._attachment_repository.list(entity_id) - return [AttachmentPostResponseSchema(**attachment.model_dump()) for attachment in attachments] + + attachments_list = [] + + for attachment in attachments: + object_key, upload_info = self._attachment_store.create_presigned_post(attachment.id, attachment) + attachments_list.append(AttachmentPostResponseSchema(**attachment.model_dump(), upload_info=upload_info)) + + return attachments_list From bc31ae875890e6de165f66b0e861bfd84867de80 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Mon, 2 Dec 2024 16:50:33 +0000 Subject: [PATCH 05/15] Add and use a new AttachmentMetadataSchema --- object_storage_api/routers/attachment.py | 8 ++++++-- object_storage_api/schemas/attachment.py | 11 +++++++++-- object_storage_api/services/attachment.py | 16 +++++++--------- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/object_storage_api/routers/attachment.py b/object_storage_api/routers/attachment.py index f8e722d..782f917 100644 --- a/object_storage_api/routers/attachment.py +++ b/object_storage_api/routers/attachment.py @@ -8,7 +8,11 @@ from fastapi import APIRouter, Depends, Query, status -from object_storage_api.schemas.attachment import AttachmentPostResponseSchema, AttachmentPostSchema +from object_storage_api.schemas.attachment import ( + AttachmentMetadataSchema, + AttachmentPostResponseSchema, + AttachmentPostSchema, +) from object_storage_api.services.attachment import AttachmentService logger = logging.getLogger() @@ -42,7 +46,7 @@ def create_attachment( def get_attachments( attachment_service: AttachmentServiceDep, entity_id: Annotated[Optional[str], Query(description="Filter attachments by entity ID")] = None, -) -> list[AttachmentPostResponseSchema]: +) -> list[AttachmentMetadataSchema]: # pylint: disable=missing-function-docstring logger.info("Getting attachments") diff --git a/object_storage_api/schemas/attachment.py b/object_storage_api/schemas/attachment.py index e1d22cf..8302fcd 100644 --- a/object_storage_api/schemas/attachment.py +++ b/object_storage_api/schemas/attachment.py @@ -29,12 +29,19 @@ class AttachmentPostUploadInfoSchema(BaseModel): fields: dict = Field(description="Form fields required for submitting the attachment file upload request") -class AttachmentPostResponseSchema(CreatedModifiedSchemaMixin, AttachmentPostSchema): +class AttachmentMetadataSchema(AttachmentPostSchema): """ - Schema model for the response to an attachment creation request. + Schema model for an attachment's metadata. """ id: str = Field(description="ID of the attachment") + + +class AttachmentPostResponseSchema(CreatedModifiedSchemaMixin, AttachmentMetadataSchema): + """ + Schema model for the response to an attachment creation request. + """ + upload_info: AttachmentPostUploadInfoSchema = Field( description="Information required to upload the attachment file" ) diff --git a/object_storage_api/services/attachment.py b/object_storage_api/services/attachment.py index 594b3e8..6883857 100644 --- a/object_storage_api/services/attachment.py +++ b/object_storage_api/services/attachment.py @@ -12,7 +12,11 @@ from object_storage_api.core.exceptions import InvalidObjectIdError from object_storage_api.models.attachment import AttachmentIn from object_storage_api.repositories.attachment import AttachmentRepo -from object_storage_api.schemas.attachment import AttachmentPostResponseSchema, AttachmentPostSchema +from object_storage_api.schemas.attachment import ( + AttachmentMetadataSchema, + AttachmentPostResponseSchema, + AttachmentPostSchema, +) from object_storage_api.stores.attachment import AttachmentStore logger = logging.getLogger() @@ -63,7 +67,7 @@ def create(self, attachment: AttachmentPostSchema) -> AttachmentPostResponseSche return AttachmentPostResponseSchema(**attachment_out.model_dump(), upload_info=upload_info) - def list(self, entity_id: Optional[str] = None) -> list[AttachmentPostResponseSchema]: + def list(self, entity_id: Optional[str] = None) -> list[AttachmentMetadataSchema]: """ Retrieve a list of attachments based on the provided filters. @@ -73,10 +77,4 @@ def list(self, entity_id: Optional[str] = None) -> list[AttachmentPostResponseSc attachments = self._attachment_repository.list(entity_id) - attachments_list = [] - - for attachment in attachments: - object_key, upload_info = self._attachment_store.create_presigned_post(attachment.id, attachment) - attachments_list.append(AttachmentPostResponseSchema(**attachment.model_dump(), upload_info=upload_info)) - - return attachments_list + return [AttachmentMetadataSchema(**attachment.model_dump()) for attachment in attachments] From 992aa581dcfb654fad602cd4595a8267fa855062 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Tue, 3 Dec 2024 18:53:28 +0000 Subject: [PATCH 06/15] Start writing tests for get attachment metadata - writing ListDSL - adding a new variable 'ATTACHMENT_GET_DATA_ALL_VALUES' into mock_data --- test/e2e/test_attachment.py | 82 +++++++++++++++++++++++++++++++++++++ test/mock_data.py | 8 +++- 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/test/e2e/test_attachment.py b/test/e2e/test_attachment.py index f30a7c0..247a5f6 100644 --- a/test/e2e/test_attachment.py +++ b/test/e2e/test_attachment.py @@ -132,3 +132,85 @@ def test_create_with_invalid_entity_id(self): self.post_attachment({**ATTACHMENT_POST_DATA_REQUIRED_VALUES_ONLY, "entity_id": "invalid-id"}) self.check_post_attachment_failed_with_detail(422, "Invalid `entity_id` given") + + +class ListDSL(CreateDSL): + """Base class for list tests.""" + + _get_response_attachment: Response + + def get_attachments(self, filters: Optional[dict] = None) -> None: + """ + Gets a list of attachments with the given filters. + + :param filters: Filters to use in the request. + """ + self._get_response_attachment = self.test_client.get("/attachments", params=filters) + + def post_test_attachments(self) -> list[dict]: + """ + Posts three attachments. The first two attachments have the same entity ID, the last attachment has a different + one. + + :return: List of dictionaries containing the expected item data returned from a get endpoint in + the form of an `AttachmentMetadataSchema`. + """ + entity_id_a, entity_id_b = (str(ObjectId()) for _ in range(2)) + + # First item + attachment_a_id = self.post_attachment( + { + **ATTACHMENT_POST_DATA_ALL_VALUES, + "entity_id": entity_id_a, + }, + ) + + # Second item + attachment_b_id = self.post_attachment( + { + **ATTACHMENT_POST_DATA_ALL_VALUES, + "entity_id": entity_id_a, + }, + ) + + # Third item + attachment_c_id = self.post_attachment( + { + **ATTACHMENT_POST_DATA_ALL_VALUES, + "entity_id": entity_id_b, + }, + ) + + return [ + { + **ATTACHMENT_GET_DATA_ALL_VALUES, + "entity_id": entity_id_a, + "id": attachment_a_id + }, + { + **ATTACHMENT_GET_DATA_ALL_VALUES, + "entity_id": entity_id_a, + "id": attachment_b_id + }, + { + **ATTACHMENT_GET_DATA_ALL_VALUES, + "entity_id": entity_id_b, + "id": attachment_c_id + } + ] + + def check_get_attachments_success(self, expected_attachments_get_data: list[dict]) -> None: + """ + Checks that a prior call to `get_attachments` gave a successful response with the expected data returned. + + :param expected_attachments_get_data: List of dictionaries containing the expected attachment data as would + be required for an `AttachmentMetadataSchema`. + """ + assert self._get_response_attachment.status_code == 200 + assert self._get_response_attachment.json() == expected_attachments_get_data + + def check_attachments_list_response_failed_with_message(self, status_code, expected_detail, obtained_detail): + """Checks the response of listing attachments failed as expected.""" + + assert self._get_response_attachment.status_code == status_code + assert obtained_detail == expected_detail diff --git a/test/mock_data.py b/test/mock_data.py index a05e979..a1e2afb 100644 --- a/test/mock_data.py +++ b/test/mock_data.py @@ -98,13 +98,17 @@ "object_key": "attachments/65df5ee771892ddcc08bd28f/65e0a624d64aaae884abaaee", } -ATTACHMENT_POST_RESPONSE_DATA_ALL_VALUES = { +ATTACHMENT_GET_DATA_ALL_VALUES = { **ATTACHMENT_POST_DATA_ALL_VALUES, **CREATED_MODIFIED_GET_DATA_EXPECTED, - **ATTACHMENT_UPLOAD_INFO_POST_RESPONSE_DATA_EXPECTED, "id": ANY, } +ATTACHMENT_POST_RESPONSE_DATA_ALL_VALUES = { + **ATTACHMENT_GET_DATA_ALL_VALUES, + **ATTACHMENT_UPLOAD_INFO_POST_RESPONSE_DATA_EXPECTED, +} + # ---------------------------- IMAGES ----------------------------- IMAGE_POST_METADATA_DATA_REQUIRED_VALUES_ONLY = { From 888e1dc7f97d4fb3c1080f4fed860f40ce9fb9e1 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Wed, 4 Dec 2024 09:30:42 +0000 Subject: [PATCH 07/15] Add get_attachments e2e tests in e2e/test_attachment - minor mock_data changes --- test/e2e/test_attachment.py | 52 +++++++++++++++++++++++++++++++++++++ test/mock_data.py | 2 +- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/test/e2e/test_attachment.py b/test/e2e/test_attachment.py index 247a5f6..08cede6 100644 --- a/test/e2e/test_attachment.py +++ b/test/e2e/test_attachment.py @@ -3,6 +3,7 @@ """ from test.mock_data import ( + ATTACHMENT_GET_DATA_ALL_VALUES, ATTACHMENT_POST_DATA_ALL_VALUES, ATTACHMENT_POST_DATA_REQUIRED_VALUES_ONLY, ATTACHMENT_POST_RESPONSE_DATA_ALL_VALUES, @@ -11,6 +12,7 @@ from typing import Optional import pytest +from bson import ObjectId import requests from fastapi.testclient import TestClient from httpx import Response @@ -214,3 +216,53 @@ def check_attachments_list_response_failed_with_message(self, status_code, expec assert self._get_response_attachment.status_code == status_code assert obtained_detail == expected_detail + + +class TestList(ListDSL): + """Tests for getting a list of attachments.""" + + def test_list_with_no_filters(self): + """ + Test getting a list of all attachments with no filters provided. + + Posts three attachments and expects all of them to be returned. + """ + + attachments = self.post_test_attachments() + self.get_attachments() + self.check_get_attachments_success(attachments) + + def test_list_with_entity_id_filter(self): + """ + Test getting a list of all attachments with an `entity_id` filter provided. + + Posts three attachments and then filter using the `entity_id`. + """ + + attachments = self.post_test_attachments() + self.get_attachments(filters={"entity_id": attachments[0]["entity_id"]}) + self.check_get_attachments_success(attachments[:2]) + + def test_list_with_entity_id_filter_with_no_matching_results(self): + """ + Test getting a list of all attachments with an `entity_id` filter provided. + + Posts three attachments and expects no results. + """ + + self.post_test_attachments() + self.get_attachments(filters={"entity_id": ObjectId()}) + self.check_get_attachments_success([]) + + def test_list_with_invalid_entity_id_filter(self): + """ + Test getting a list of all attachments with an invalid `entity_id` filter provided. + + Posts three attachments and expects a 422 status code. + """ + + self. post_test_attachments() + self.get_attachments(filters={"entity_id": False}) + self.check_attachments_list_response_failed_with_message( + 422, "Invalid ID given", self._get_response_attachment.json()["detail"] + ) diff --git a/test/mock_data.py b/test/mock_data.py index a1e2afb..cd1daae 100644 --- a/test/mock_data.py +++ b/test/mock_data.py @@ -100,12 +100,12 @@ ATTACHMENT_GET_DATA_ALL_VALUES = { **ATTACHMENT_POST_DATA_ALL_VALUES, - **CREATED_MODIFIED_GET_DATA_EXPECTED, "id": ANY, } ATTACHMENT_POST_RESPONSE_DATA_ALL_VALUES = { **ATTACHMENT_GET_DATA_ALL_VALUES, + **CREATED_MODIFIED_GET_DATA_EXPECTED, **ATTACHMENT_UPLOAD_INFO_POST_RESPONSE_DATA_EXPECTED, } From 76161a280d3a77620ab454fe2925792f00ee3420 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Wed, 4 Dec 2024 15:16:27 +0000 Subject: [PATCH 08/15] Add repositories unit tests when listing get attachments #25 --- test/unit/repositories/test_attachment.py | 72 +++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/test/unit/repositories/test_attachment.py b/test/unit/repositories/test_attachment.py index 69eaed7..e843a6c 100644 --- a/test/unit/repositories/test_attachment.py +++ b/test/unit/repositories/test_attachment.py @@ -4,9 +4,11 @@ from test.mock_data import ATTACHMENT_IN_DATA_ALL_VALUES from test.unit.repositories.conftest import RepositoryTestHelpers +from typing import Optional from unittest.mock import MagicMock, Mock import pytest +from bson import ObjectId from object_storage_api.models.attachment import AttachmentIn, AttachmentOut from object_storage_api.repositories.attachment import AttachmentRepo @@ -86,3 +88,73 @@ def test_create(self): self.mock_create(ATTACHMENT_IN_DATA_ALL_VALUES) self.call_create() self.check_create_success() + + +class ListDSL(AttachmentRepoDSL): + """Base class for `list` tests.""" + + _expected_attachment_out: list[AttachmentOut] + _entity_id_filter: Optional[str] + _obtained_attachment_out: list[AttachmentOut] + + def mock_list(self, attachment_in_data: list[dict]) -> None: + """ + Mocks database methods appropriately to test the `list` repo method. + + :param attachment_in_data: List of dictionaries containing the attachment data as would be required for an + `AttachmentIn` database model (i.e. no ID or created and modified times required). + """ + self._expected_attachment_out = [ + AttachmentOut(**AttachmentIn(**attachment_in_data).model_dump()) + for attachment_in_data in attachment_in_data + ] + + RepositoryTestHelpers.mock_find( + self.attachments_collection, + [ + attachment_out.model_dump() + for attachment_out in self._expected_attachment_out + ] + ) + + def call_list(self, entity_id: Optional[str] = None) -> None: + """ + Calls the `AttachmentRepo` `list method` method. + + :param entity_id: The ID of the entity to filter attachments by. + """ + self._entity_id_filter = entity_id + self._obtained_attachment_out = self.attachment_repository.list( + session=self.mock_session, entity_id=entity_id + ) + + def check_list_success(self) -> None: + """Checks that a prior call to `call_list` worked as expected.""" + expected_query = {} + if self._entity_id_filter is not None: + expected_query["entity_id"] = ObjectId(self._entity_id_filter) + + self.attachments_collection.find.assert_called_once_with(expected_query, session=self.mock_session) + assert self._obtained_attachment_out == self._expected_attachment_out + + +class TestList(ListDSL): + """Tests for listing attachments.""" + + def test_list(self): + """Test listing all attachments.""" + self.mock_list([ATTACHMENT_IN_DATA_ALL_VALUES]) + self.call_list() + self.check_list_success() + + def test_list_with_no_results(self): + """Test listing all attachments returning no results.""" + self.mock_list([]) + self.call_list() + self.check_list_success() + + def test_list_with_entity_id(self): + """Test listing all attachments with an `entity_id` argument.""" + self.mock_list([ATTACHMENT_IN_DATA_ALL_VALUES]) + self.call_list(entity_id=ATTACHMENT_IN_DATA_ALL_VALUES["entity_id"]) + self.check_list_success() From 0bd2fefbec220f1140e932a95a08560265ecaec8 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Wed, 4 Dec 2024 16:01:44 +0000 Subject: [PATCH 09/15] Add services unit tests when listing attachments #25 --- test/unit/services/test_attachment.py | 47 ++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/test/unit/services/test_attachment.py b/test/unit/services/test_attachment.py index 2384235..aee5a5d 100644 --- a/test/unit/services/test_attachment.py +++ b/test/unit/services/test_attachment.py @@ -2,7 +2,8 @@ Unit tests for the `AttachmentService` service. """ -from test.mock_data import ATTACHMENT_POST_DATA_ALL_VALUES +from test.mock_data import ATTACHMENT_IN_DATA_ALL_VALUES, ATTACHMENT_POST_DATA_ALL_VALUES +from typing import List, Optional from unittest.mock import MagicMock, Mock, patch import pytest @@ -11,6 +12,7 @@ from object_storage_api.core.exceptions import InvalidObjectIdError from object_storage_api.models.attachment import AttachmentIn, AttachmentOut from object_storage_api.schemas.attachment import ( + AttachmentMetadataSchema, AttachmentPostResponseSchema, AttachmentPostSchema, AttachmentPostUploadInfoSchema, @@ -153,3 +155,46 @@ def test_create_with_invalid_entity_id(self): self.mock_create({**ATTACHMENT_POST_DATA_ALL_VALUES, "entity_id": "invalid-id"}) self.call_create_expecting_error(InvalidObjectIdError) self.check_create_failed_with_exception("Invalid ObjectId value 'invalid-id'") + + +class ListDSL(AttachmentServiceDSL): + """Base class for `list` tests.""" + + _entity_id_filter: Optional[str] + _expected_attachments: List[AttachmentMetadataSchema] + _obtained_attachments: List[AttachmentMetadataSchema] + + def mock_list(self) -> None: + """Mocks repo methods appropriately to test the `list` service method.""" + + # Just returns the result after converting it to the schemas currently, so actual value doesn't matter here. + attachments_out = [AttachmentOut(**AttachmentIn(**ATTACHMENT_IN_DATA_ALL_VALUES).model_dump())] + self.mock_attachment_repository.list.return_value = attachments_out + self._expected_attachments = [ + AttachmentMetadataSchema(**attachment_out.model_dump()) + for attachment_out in attachments_out + ] + + def call_list(self, entity_id: Optional[str] = None) -> None: + """ + Calls the `AttachmentService` `list` method. + + :param entity_id: The ID of the entity to filter attachments by. + """ + self._entity_id_filter = entity_id + self._obtained_attachments = self.attachment_service.list(entity_id=entity_id) + + def check_list_success(self) -> None: + """Checks that a prior call to `call_list` worked as expected.""" + self.mock_attachment_repository.list.assert_called_once_with(self._entity_id_filter) + assert self._obtained_attachments == self._expected_attachments + + +class TestList(ListDSL): + """Tests for listing attachments.""" + + def test_list(self): + """Test listing attachments.""" + self.mock_list() + self.call_list(entity_id=str(ObjectId())) + self.check_list_success() From 36cc82832820a7781f3dea60ca3bb150acc67c6b Mon Sep 17 00:00:00 2001 From: rowan04 Date: Thu, 5 Dec 2024 10:07:31 +0000 Subject: [PATCH 10/15] Black auto reformatter changes to pass action checks #25 --- object_storage_api/repositories/attachment.py | 4 ++-- object_storage_api/routers/attachment.py | 1 + test/e2e/test_attachment.py | 20 ++++--------------- test/unit/repositories/test_attachment.py | 9 ++------- test/unit/services/test_attachment.py | 3 +-- 5 files changed, 10 insertions(+), 27 deletions(-) diff --git a/object_storage_api/repositories/attachment.py b/object_storage_api/repositories/attachment.py index 200d299..90a3989 100644 --- a/object_storage_api/repositories/attachment.py +++ b/object_storage_api/repositories/attachment.py @@ -69,13 +69,13 @@ def list(self, entity_id: Optional[str], session: ClientSession = None) -> list[ query = {} if entity_id is not None: query["entity_id"] = CustomObjectId(entity_id) - + message = "Retrieving all attachments from the database" if not query: logger.info(message) else: logger.info("%s matching the provided filter(s)", message) logger.debug("Provided filter(s): %s", query) - + attachments = self._attachments_collection.find(query, session=session) return [AttachmentOut(**attachment) for attachment in attachments] diff --git a/object_storage_api/routers/attachment.py b/object_storage_api/routers/attachment.py index 782f917..7caa908 100644 --- a/object_storage_api/routers/attachment.py +++ b/object_storage_api/routers/attachment.py @@ -38,6 +38,7 @@ def create_attachment( return attachment_service.create(attachment) + @router.get( path="", summary="Get attachments", diff --git a/test/e2e/test_attachment.py b/test/e2e/test_attachment.py index 08cede6..e3bb6f6 100644 --- a/test/e2e/test_attachment.py +++ b/test/e2e/test_attachment.py @@ -184,21 +184,9 @@ def post_test_attachments(self) -> list[dict]: ) return [ - { - **ATTACHMENT_GET_DATA_ALL_VALUES, - "entity_id": entity_id_a, - "id": attachment_a_id - }, - { - **ATTACHMENT_GET_DATA_ALL_VALUES, - "entity_id": entity_id_a, - "id": attachment_b_id - }, - { - **ATTACHMENT_GET_DATA_ALL_VALUES, - "entity_id": entity_id_b, - "id": attachment_c_id - } + {**ATTACHMENT_GET_DATA_ALL_VALUES, "entity_id": entity_id_a, "id": attachment_a_id}, + {**ATTACHMENT_GET_DATA_ALL_VALUES, "entity_id": entity_id_a, "id": attachment_b_id}, + {**ATTACHMENT_GET_DATA_ALL_VALUES, "entity_id": entity_id_b, "id": attachment_c_id}, ] def check_get_attachments_success(self, expected_attachments_get_data: list[dict]) -> None: @@ -261,7 +249,7 @@ def test_list_with_invalid_entity_id_filter(self): Posts three attachments and expects a 422 status code. """ - self. post_test_attachments() + self.post_test_attachments() self.get_attachments(filters={"entity_id": False}) self.check_attachments_list_response_failed_with_message( 422, "Invalid ID given", self._get_response_attachment.json()["detail"] diff --git a/test/unit/repositories/test_attachment.py b/test/unit/repositories/test_attachment.py index e843a6c..18d8fbc 100644 --- a/test/unit/repositories/test_attachment.py +++ b/test/unit/repositories/test_attachment.py @@ -111,10 +111,7 @@ def mock_list(self, attachment_in_data: list[dict]) -> None: RepositoryTestHelpers.mock_find( self.attachments_collection, - [ - attachment_out.model_dump() - for attachment_out in self._expected_attachment_out - ] + [attachment_out.model_dump() for attachment_out in self._expected_attachment_out], ) def call_list(self, entity_id: Optional[str] = None) -> None: @@ -124,9 +121,7 @@ def call_list(self, entity_id: Optional[str] = None) -> None: :param entity_id: The ID of the entity to filter attachments by. """ self._entity_id_filter = entity_id - self._obtained_attachment_out = self.attachment_repository.list( - session=self.mock_session, entity_id=entity_id - ) + self._obtained_attachment_out = self.attachment_repository.list(session=self.mock_session, entity_id=entity_id) def check_list_success(self) -> None: """Checks that a prior call to `call_list` worked as expected.""" diff --git a/test/unit/services/test_attachment.py b/test/unit/services/test_attachment.py index aee5a5d..b56ec46 100644 --- a/test/unit/services/test_attachment.py +++ b/test/unit/services/test_attachment.py @@ -171,8 +171,7 @@ def mock_list(self) -> None: attachments_out = [AttachmentOut(**AttachmentIn(**ATTACHMENT_IN_DATA_ALL_VALUES).model_dump())] self.mock_attachment_repository.list.return_value = attachments_out self._expected_attachments = [ - AttachmentMetadataSchema(**attachment_out.model_dump()) - for attachment_out in attachments_out + AttachmentMetadataSchema(**attachment_out.model_dump()) for attachment_out in attachments_out ] def call_list(self, entity_id: Optional[str] = None) -> None: From 0273a0b72b33b2466b1b5da0b1bdc186733d21d1 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Thu, 5 Dec 2024 14:33:17 +0000 Subject: [PATCH 11/15] Disable pylint duplicate code check for repositories attachments and images to pass action checks #25 --- object_storage_api/repositories/attachment.py | 3 +++ object_storage_api/repositories/image.py | 3 +++ test/unit/repositories/test_attachment.py | 3 +++ test/unit/repositories/test_image.py | 3 +++ 4 files changed, 12 insertions(+) diff --git a/object_storage_api/repositories/attachment.py b/object_storage_api/repositories/attachment.py index 90a3989..aabb4dc 100644 --- a/object_storage_api/repositories/attachment.py +++ b/object_storage_api/repositories/attachment.py @@ -2,6 +2,9 @@ Module for providing a repository for managing attachments in a MongoDB database. """ +# Expect some duplicate code inside tests as the tests for the different entities can be very similar +# pylint: disable=duplicate-code + import logging from typing import Optional diff --git a/object_storage_api/repositories/image.py b/object_storage_api/repositories/image.py index 09d4191..8aa2a55 100644 --- a/object_storage_api/repositories/image.py +++ b/object_storage_api/repositories/image.py @@ -2,6 +2,9 @@ Module for providing a repository for managing images in a MongoDB database. """ +# Expect some duplicate code inside tests as the tests for the different entities can be very similar +# pylint: disable=duplicate-code + import logging from typing import Optional diff --git a/test/unit/repositories/test_attachment.py b/test/unit/repositories/test_attachment.py index 18d8fbc..7c1c397 100644 --- a/test/unit/repositories/test_attachment.py +++ b/test/unit/repositories/test_attachment.py @@ -2,6 +2,9 @@ Unit tests for the `AttachmentRepo` repository. """ +# Expect some duplicate code inside tests as the tests for the different entities can be very similar +# pylint: disable=duplicate-code + from test.mock_data import ATTACHMENT_IN_DATA_ALL_VALUES from test.unit.repositories.conftest import RepositoryTestHelpers from typing import Optional diff --git a/test/unit/repositories/test_image.py b/test/unit/repositories/test_image.py index c50fdcc..e7db725 100644 --- a/test/unit/repositories/test_image.py +++ b/test/unit/repositories/test_image.py @@ -2,6 +2,9 @@ Unit tests for the `ImageRepo` repository. """ +# Expect some duplicate code inside tests as the tests for the different entities can be very similar +# pylint: disable=duplicate-code + from test.mock_data import IMAGE_IN_DATA_ALL_VALUES from test.unit.repositories.conftest import RepositoryTestHelpers from typing import Optional From 9b0d4ce30627c2a07a723110cbe805a9d45e127c Mon Sep 17 00:00:00 2001 From: rowan04 Date: Mon, 9 Dec 2024 13:02:19 +0000 Subject: [PATCH 12/15] Addressing code review comments #25 - rewording comments - moving duplicate code flags - rename function - for both attachments and images --- object_storage_api/repositories/attachment.py | 8 +++++--- object_storage_api/repositories/image.py | 8 +++++--- test/e2e/test_attachment.py | 6 +++--- test/e2e/test_image.py | 8 ++++---- test/unit/repositories/test_attachment.py | 9 +++++---- test/unit/repositories/test_image.py | 9 +++++---- 6 files changed, 27 insertions(+), 21 deletions(-) diff --git a/object_storage_api/repositories/attachment.py b/object_storage_api/repositories/attachment.py index aabb4dc..3b3502b 100644 --- a/object_storage_api/repositories/attachment.py +++ b/object_storage_api/repositories/attachment.py @@ -2,9 +2,6 @@ Module for providing a repository for managing attachments in a MongoDB database. """ -# Expect some duplicate code inside tests as the tests for the different entities can be very similar -# pylint: disable=duplicate-code - import logging from typing import Optional @@ -69,6 +66,9 @@ def list(self, entity_id: Optional[str], session: ClientSession = None) -> list[ :return: List of attachments or an empty list if no attachments are retrieved. """ + # There is some duplicate code here, due to the attachments and images methods being very similar + # pylint: disable=duplicate-code + query = {} if entity_id is not None: query["entity_id"] = CustomObjectId(entity_id) @@ -80,5 +80,7 @@ def list(self, entity_id: Optional[str], session: ClientSession = None) -> list[ logger.info("%s matching the provided filter(s)", message) logger.debug("Provided filter(s): %s", query) + # pylint: enable=duplicate-code + attachments = self._attachments_collection.find(query, session=session) return [AttachmentOut(**attachment) for attachment in attachments] diff --git a/object_storage_api/repositories/image.py b/object_storage_api/repositories/image.py index 8aa2a55..008e3c8 100644 --- a/object_storage_api/repositories/image.py +++ b/object_storage_api/repositories/image.py @@ -2,9 +2,6 @@ Module for providing a repository for managing images in a MongoDB database. """ -# Expect some duplicate code inside tests as the tests for the different entities can be very similar -# pylint: disable=duplicate-code - import logging from typing import Optional @@ -70,6 +67,9 @@ def list(self, entity_id: Optional[str], primary: Optional[bool], session: Clien :return: List of images or an empty list if no images are retrieved. """ + # There is some duplicate code here, due to the attachments and images methods being very similar + # pylint: disable=duplicate-code + query = {} if entity_id is not None: query["entity_id"] = CustomObjectId(entity_id) @@ -83,5 +83,7 @@ def list(self, entity_id: Optional[str], primary: Optional[bool], session: Clien logger.info("%s matching the provided filter(s)", message) logger.debug("Provided filter(s): %s", query) + # pylint: enable=duplicate-code + images = self._images_collection.find(query, session=session) return [ImageOut(**image) for image in images] diff --git a/test/e2e/test_attachment.py b/test/e2e/test_attachment.py index e3bb6f6..eded8ec 100644 --- a/test/e2e/test_attachment.py +++ b/test/e2e/test_attachment.py @@ -199,8 +199,8 @@ def check_get_attachments_success(self, expected_attachments_get_data: list[dict assert self._get_response_attachment.status_code == 200 assert self._get_response_attachment.json() == expected_attachments_get_data - def check_attachments_list_response_failed_with_message(self, status_code, expected_detail, obtained_detail): - """Checks the response of listing attachments failed as expected.""" + def check_get_attachments_failed_with_message(self, status_code, expected_detail, obtained_detail): + """Checks the response of listing attachments failed with the expected message.""" assert self._get_response_attachment.status_code == status_code assert obtained_detail == expected_detail @@ -251,6 +251,6 @@ def test_list_with_invalid_entity_id_filter(self): self.post_test_attachments() self.get_attachments(filters={"entity_id": False}) - self.check_attachments_list_response_failed_with_message( + self.check_get_attachments_failed_with_message( 422, "Invalid ID given", self._get_response_attachment.json()["detail"] ) diff --git a/test/e2e/test_image.py b/test/e2e/test_image.py index f070388..7437d11 100644 --- a/test/e2e/test_image.py +++ b/test/e2e/test_image.py @@ -171,8 +171,8 @@ def check_get_images_success(self, expected_images_get_data: list[dict]) -> None assert self._get_response_image.status_code == 200 assert self._get_response_image.json() == expected_images_get_data - def check_images_list_response_failed_with_message(self, status_code, expected_detail, obtained_detail): - """Checks the response of listing images failed as expected.""" + def check_get_images_failed_with_message(self, status_code, expected_detail, obtained_detail): + """Checks the response of listing images failed with the expected message.""" assert self._get_response_image.status_code == status_code assert obtained_detail == expected_detail @@ -221,7 +221,7 @@ def test_list_with_invalid_entity_id_filter(self): """ self.post_test_images() self.get_images(filters={"entity_id": False}) - self.check_images_list_response_failed_with_message( + self.check_get_images_failed_with_message( 422, "Invalid ID given", self._get_response_image.json()["detail"] ) @@ -254,7 +254,7 @@ def test_list_with_invalid_primary_filter(self): """ self.post_test_images() self.get_images(filters={"primary": str(ObjectId())}) - self.check_images_list_response_failed_with_message( + self.check_get_images_failed_with_message( 422, "Input should be a valid boolean, unable to interpret input", self._get_response_image.json()["detail"][0]["msg"], diff --git a/test/unit/repositories/test_attachment.py b/test/unit/repositories/test_attachment.py index 7c1c397..a06f4dd 100644 --- a/test/unit/repositories/test_attachment.py +++ b/test/unit/repositories/test_attachment.py @@ -2,9 +2,6 @@ Unit tests for the `AttachmentRepo` repository. """ -# Expect some duplicate code inside tests as the tests for the different entities can be very similar -# pylint: disable=duplicate-code - from test.mock_data import ATTACHMENT_IN_DATA_ALL_VALUES from test.unit.repositories.conftest import RepositoryTestHelpers from typing import Optional @@ -105,7 +102,7 @@ def mock_list(self, attachment_in_data: list[dict]) -> None: Mocks database methods appropriately to test the `list` repo method. :param attachment_in_data: List of dictionaries containing the attachment data as would be required for an - `AttachmentIn` database model (i.e. no ID or created and modified times required). + `AttachmentIn` database model (i.e. no created and modified times required). """ self._expected_attachment_out = [ AttachmentOut(**AttachmentIn(**attachment_in_data).model_dump()) @@ -135,6 +132,8 @@ def check_list_success(self) -> None: self.attachments_collection.find.assert_called_once_with(expected_query, session=self.mock_session) assert self._obtained_attachment_out == self._expected_attachment_out +# Expect some duplicate code inside tests as the tests for the different entities can be very similar +# pylint: disable=duplicate-code class TestList(ListDSL): """Tests for listing attachments.""" @@ -156,3 +155,5 @@ def test_list_with_entity_id(self): self.mock_list([ATTACHMENT_IN_DATA_ALL_VALUES]) self.call_list(entity_id=ATTACHMENT_IN_DATA_ALL_VALUES["entity_id"]) self.check_list_success() + +# pylint: enable=duplicate-code diff --git a/test/unit/repositories/test_image.py b/test/unit/repositories/test_image.py index e7db725..36a54a2 100644 --- a/test/unit/repositories/test_image.py +++ b/test/unit/repositories/test_image.py @@ -2,9 +2,6 @@ Unit tests for the `ImageRepo` repository. """ -# Expect some duplicate code inside tests as the tests for the different entities can be very similar -# pylint: disable=duplicate-code - from test.mock_data import IMAGE_IN_DATA_ALL_VALUES from test.unit.repositories.conftest import RepositoryTestHelpers from typing import Optional @@ -104,7 +101,7 @@ def mock_list(self, image_in_data: list[dict]) -> None: Mocks database methods appropriately to test the `list` repo method. :param image_in_data: List of dictionaries containing the image data as would be required for an - `ImageIn` database model (i.e. no ID or created and modified times required). + `ImageIn` database model (i.e. no created and modified times required). """ self._expected_image_out = [ ImageOut(**ImageIn(**image_in_data).model_dump()) for image_in_data in image_in_data @@ -137,6 +134,8 @@ def check_list_success(self) -> None: self.images_collection.find.assert_called_once_with(expected_query, session=self.mock_session) assert self._obtained_image_out == self._expected_image_out +# Expect some duplicate code inside tests as the tests for the different entities can be very similar +# pylint: disable=duplicate-code class TestList(ListDSL): """Tests for listing images.""" @@ -170,3 +169,5 @@ def test_list_with_primary_and_entity_id(self): self.mock_list([IMAGE_IN_DATA_ALL_VALUES]) self.call_list(primary=True, entity_id=IMAGE_IN_DATA_ALL_VALUES["entity_id"]) self.check_list_success() + +# pylint: enable=duplicate-code From 51fd223a949f4522c3144b587d87268b00956683 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Mon, 9 Dec 2024 15:49:39 +0000 Subject: [PATCH 13/15] Black auto reformatter changes #25 --- test/e2e/test_image.py | 4 +--- test/unit/repositories/test_attachment.py | 3 +++ test/unit/repositories/test_image.py | 3 +++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/test/e2e/test_image.py b/test/e2e/test_image.py index 7437d11..7879f78 100644 --- a/test/e2e/test_image.py +++ b/test/e2e/test_image.py @@ -221,9 +221,7 @@ def test_list_with_invalid_entity_id_filter(self): """ self.post_test_images() self.get_images(filters={"entity_id": False}) - self.check_get_images_failed_with_message( - 422, "Invalid ID given", self._get_response_image.json()["detail"] - ) + self.check_get_images_failed_with_message(422, "Invalid ID given", self._get_response_image.json()["detail"]) def test_list_with_primary_filter(self): """ diff --git a/test/unit/repositories/test_attachment.py b/test/unit/repositories/test_attachment.py index a06f4dd..6f1876e 100644 --- a/test/unit/repositories/test_attachment.py +++ b/test/unit/repositories/test_attachment.py @@ -132,9 +132,11 @@ def check_list_success(self) -> None: self.attachments_collection.find.assert_called_once_with(expected_query, session=self.mock_session) assert self._obtained_attachment_out == self._expected_attachment_out + # Expect some duplicate code inside tests as the tests for the different entities can be very similar # pylint: disable=duplicate-code + class TestList(ListDSL): """Tests for listing attachments.""" @@ -156,4 +158,5 @@ def test_list_with_entity_id(self): self.call_list(entity_id=ATTACHMENT_IN_DATA_ALL_VALUES["entity_id"]) self.check_list_success() + # pylint: enable=duplicate-code diff --git a/test/unit/repositories/test_image.py b/test/unit/repositories/test_image.py index 36a54a2..3eebcb8 100644 --- a/test/unit/repositories/test_image.py +++ b/test/unit/repositories/test_image.py @@ -134,9 +134,11 @@ def check_list_success(self) -> None: self.images_collection.find.assert_called_once_with(expected_query, session=self.mock_session) assert self._obtained_image_out == self._expected_image_out + # Expect some duplicate code inside tests as the tests for the different entities can be very similar # pylint: disable=duplicate-code + class TestList(ListDSL): """Tests for listing images.""" @@ -170,4 +172,5 @@ def test_list_with_primary_and_entity_id(self): self.call_list(primary=True, entity_id=IMAGE_IN_DATA_ALL_VALUES["entity_id"]) self.check_list_success() + # pylint: enable=duplicate-code From a3911ffb92705acbc490d1e43e6f6462e77625d1 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Mon, 9 Dec 2024 15:50:24 +0000 Subject: [PATCH 14/15] Move invalid entity_id error handling for list attachments into repositories layer - also renames response message --- object_storage_api/repositories/attachment.py | 10 +++++++++- test/e2e/test_attachment.py | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/object_storage_api/repositories/attachment.py b/object_storage_api/repositories/attachment.py index 3b3502b..d218cd7 100644 --- a/object_storage_api/repositories/attachment.py +++ b/object_storage_api/repositories/attachment.py @@ -10,6 +10,7 @@ from object_storage_api.core.custom_object_id import CustomObjectId from object_storage_api.core.database import DatabaseDep +from object_storage_api.core.exceptions import InvalidObjectIdError from object_storage_api.models.attachment import AttachmentIn, AttachmentOut logger = logging.getLogger() @@ -64,14 +65,21 @@ def list(self, entity_id: Optional[str], session: ClientSession = None) -> list[ :param session: PyMongo ClientSession to use for database operations. :param entity_id: The ID of the entity to filter attachments by. :return: List of attachments or an empty list if no attachments are retrieved. + :raises InvalidObjectIdError: If the supplied `entity_id` is invalid. """ # There is some duplicate code here, due to the attachments and images methods being very similar # pylint: disable=duplicate-code query = {} + if entity_id is not None: - query["entity_id"] = CustomObjectId(entity_id) + try: + query["entity_id"] = CustomObjectId(entity_id) + except InvalidObjectIdError as exc: + exc.status_code = 422 + exc.response_detail = "Attachment not found" + raise exc message = "Retrieving all attachments from the database" if not query: diff --git a/test/e2e/test_attachment.py b/test/e2e/test_attachment.py index eded8ec..7c64e88 100644 --- a/test/e2e/test_attachment.py +++ b/test/e2e/test_attachment.py @@ -252,5 +252,5 @@ def test_list_with_invalid_entity_id_filter(self): self.post_test_attachments() self.get_attachments(filters={"entity_id": False}) self.check_get_attachments_failed_with_message( - 422, "Invalid ID given", self._get_response_attachment.json()["detail"] + 422, "Attachment not found", self._get_response_attachment.json()["detail"] ) From f645bb89009a91ecdbd8c1a4292827e778ab9af8 Mon Sep 17 00:00:00 2001 From: rowan04 Date: Mon, 9 Dec 2024 16:36:44 +0000 Subject: [PATCH 15/15] Revert "Move invalid entity_id error handling for list attachments into repositories layer" This reverts commit a3911ffb92705acbc490d1e43e6f6462e77625d1. --- object_storage_api/repositories/attachment.py | 10 +--------- test/e2e/test_attachment.py | 2 +- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/object_storage_api/repositories/attachment.py b/object_storage_api/repositories/attachment.py index d218cd7..3b3502b 100644 --- a/object_storage_api/repositories/attachment.py +++ b/object_storage_api/repositories/attachment.py @@ -10,7 +10,6 @@ from object_storage_api.core.custom_object_id import CustomObjectId from object_storage_api.core.database import DatabaseDep -from object_storage_api.core.exceptions import InvalidObjectIdError from object_storage_api.models.attachment import AttachmentIn, AttachmentOut logger = logging.getLogger() @@ -65,21 +64,14 @@ def list(self, entity_id: Optional[str], session: ClientSession = None) -> list[ :param session: PyMongo ClientSession to use for database operations. :param entity_id: The ID of the entity to filter attachments by. :return: List of attachments or an empty list if no attachments are retrieved. - :raises InvalidObjectIdError: If the supplied `entity_id` is invalid. """ # There is some duplicate code here, due to the attachments and images methods being very similar # pylint: disable=duplicate-code query = {} - if entity_id is not None: - try: - query["entity_id"] = CustomObjectId(entity_id) - except InvalidObjectIdError as exc: - exc.status_code = 422 - exc.response_detail = "Attachment not found" - raise exc + query["entity_id"] = CustomObjectId(entity_id) message = "Retrieving all attachments from the database" if not query: diff --git a/test/e2e/test_attachment.py b/test/e2e/test_attachment.py index 7c64e88..eded8ec 100644 --- a/test/e2e/test_attachment.py +++ b/test/e2e/test_attachment.py @@ -252,5 +252,5 @@ def test_list_with_invalid_entity_id_filter(self): self.post_test_attachments() self.get_attachments(filters={"entity_id": False}) self.check_get_attachments_failed_with_message( - 422, "Attachment not found", self._get_response_attachment.json()["detail"] + 422, "Invalid ID given", self._get_response_attachment.json()["detail"] )