From c66816974a23d5051074dbc179d2892743dbbd93 Mon Sep 17 00:00:00 2001 From: abbas-khan10 <127417949+abbas-khan10@users.noreply.github.com> Date: Thu, 21 Mar 2024 15:07:19 +0000 Subject: [PATCH] [PATCH] PRMDR-738 - Add DynamoDb Query Filter module (#325) * add Dynamo Filter module * remove ttl from metadata fields list * remove redundant field from metadata fields list * change lastupdated data type * refactor query builder * fix tests * add dynamo exceptions to query filter builder * add exception tests --- lambdas/enums/dynamo_filter.py | 15 ++ lambdas/enums/metadata_field_names.py | 5 +- lambdas/models/document_reference.py | 2 +- lambdas/services/base/dynamo_service.py | 23 +-- .../document_reference_search_service.py | 14 +- lambdas/services/document_service.py | 15 +- .../unit/enums/test_metadata_field_names.py | 4 +- .../unit/helpers/data/dynamo_responses.py | 12 +- .../unit/services/base/test_dynamo_service.py | 37 ++-- .../test_document_reference_search_service.py | 2 +- .../unit/services/test_document_service.py | 59 ++++--- .../test_lloyd_george_stitch_service.py | 2 +- .../utils/test_dynamo_query_filter_builder.py | 158 ++++++++++++++++++ lambdas/tests/unit/utils/test_dynamo_utils.py | 82 +-------- lambdas/utils/dynamo_query_filter_builder.py | 51 ++++++ lambdas/utils/dynamo_utils.py | 38 ----- 16 files changed, 322 insertions(+), 197 deletions(-) create mode 100644 lambdas/enums/dynamo_filter.py create mode 100644 lambdas/tests/unit/utils/test_dynamo_query_filter_builder.py create mode 100644 lambdas/utils/dynamo_query_filter_builder.py diff --git a/lambdas/enums/dynamo_filter.py b/lambdas/enums/dynamo_filter.py new file mode 100644 index 000000000..1ce00910b --- /dev/null +++ b/lambdas/enums/dynamo_filter.py @@ -0,0 +1,15 @@ +from enum import Enum + + +class AttributeOperator(Enum): + EQUAL = "eq" + NOT_EQUAL = "ne" + GREATER_THAN = "gt" + GREATER_OR_EQUAL = "gte" + LESS_THAN = "lt" + LESS_THAN_OR_EQUAL = "lte" + + +class ConditionOperator(Enum): + OR = "|" + AND = "&" diff --git a/lambdas/enums/metadata_field_names.py b/lambdas/enums/metadata_field_names.py index e956da183..b777f94a4 100755 --- a/lambdas/enums/metadata_field_names.py +++ b/lambdas/enums/metadata_field_names.py @@ -10,7 +10,6 @@ class DocumentReferenceMetadataFields(Enum): FILE_LOCATION = "FileLocation" NHS_NUMBER = "NhsNumber" TTL = "TTL" - TYPE = "Type" VIRUS_SCANNER_RESULT = "VirusScannerResult" CURRENT_GP_ODS = "CurrentGpOds" UPLOADED = "Uploaded" @@ -19,7 +18,9 @@ class DocumentReferenceMetadataFields(Enum): @staticmethod def list() -> list[str]: - return [str(field.value) for field in DocumentReferenceMetadataFields] + fields = [str(field.value) for field in DocumentReferenceMetadataFields] + fields.remove(DocumentReferenceMetadataFields.TTL.value) + return fields class DocumentZipTraceFields(Enum): diff --git a/lambdas/models/document_reference.py b/lambdas/models/document_reference.py index c0635c8c8..6eee25707 100644 --- a/lambdas/models/document_reference.py +++ b/lambdas/models/document_reference.py @@ -44,7 +44,7 @@ class DocumentReference(BaseModel): uploaded: bool = Field(alias=str(DocumentReferenceMetadataFields.UPLOADED.value)) uploading: bool = Field(alias=str(DocumentReferenceMetadataFields.UPLOADING.value)) - last_updated: str = Field( + last_updated: int = Field( alias=str(DocumentReferenceMetadataFields.LAST_UPDATED.value), serialization_alias="lastUpdated", ) diff --git a/lambdas/services/base/dynamo_service.py b/lambdas/services/base/dynamo_service.py index 7b1825880..94adc1531 100644 --- a/lambdas/services/base/dynamo_service.py +++ b/lambdas/services/base/dynamo_service.py @@ -1,9 +1,8 @@ import boto3 -from boto3.dynamodb.conditions import Key +from boto3.dynamodb.conditions import Attr, ConditionBase, Key from botocore.exceptions import ClientError from utils.audit_logging_setup import LoggingService from utils.dynamo_utils import ( - create_attribute_filter, create_expression_attribute_values, create_expressions, create_update_expression, @@ -40,8 +39,8 @@ def query_with_requested_fields( index_name, search_key, search_condition: str, - requested_fields: list = None, - filtered_fields: dict = None, + requested_fields: list[str] = None, + query_filter: Attr | ConditionBase = None, ): try: table = self.get_table(table_name) @@ -52,29 +51,19 @@ def query_with_requested_fields( "Unable to query DynamoDB with empty requested fields" ) - projection_expression, expression_attribute_names = create_expressions( - requested_fields - ) + projection_expression = ",".join(requested_fields) - if not filtered_fields: + if not query_filter: results = table.query( IndexName=index_name, KeyConditionExpression=Key(search_key).eq(search_condition), - ExpressionAttributeNames=expression_attribute_names, ProjectionExpression=projection_expression, ) else: - fields_filter = create_attribute_filter(filtered_fields) - expression_attribute_values = create_expression_attribute_values( - filtered_fields - ) - results = table.query( IndexName=index_name, KeyConditionExpression=Key(search_key).eq(search_condition), - FilterExpression=fields_filter, - ExpressionAttributeNames=expression_attribute_names, - ExpressionAttributeValues=expression_attribute_values, + FilterExpression=query_filter, ProjectionExpression=projection_expression, ) diff --git a/lambdas/services/document_reference_search_service.py b/lambdas/services/document_reference_search_service.py index 245be35cc..6ce6f4232 100644 --- a/lambdas/services/document_reference_search_service.py +++ b/lambdas/services/document_reference_search_service.py @@ -3,12 +3,14 @@ from json import JSONDecodeError from botocore.exceptions import ClientError +from enums.dynamo_filter import AttributeOperator from enums.lambda_error import LambdaError from enums.metadata_field_names import DocumentReferenceMetadataFields from models.document_reference import DocumentReference from pydantic import ValidationError from services.document_service import DocumentService from utils.audit_logging_setup import LoggingService +from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder from utils.exceptions import DynamoServiceException from utils.lambda_exceptions import DocumentRefSearchException @@ -19,16 +21,24 @@ class DocumentReferenceSearchService(DocumentService): def get_document_references(self, nhs_number: str): try: list_of_table_names = json.loads(os.environ["DYNAMODB_TABLE_LIST"]) - results: list[dict] = [] + + filter_builder = DynamoQueryFilterBuilder() + delete_filter_expression = filter_builder.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator=AttributeOperator.EQUAL, + filter_value="", + ).build() + for table_name in list_of_table_names: logger.info(f"Searching for results in {table_name}") + documents: list[ DocumentReference ] = self.fetch_documents_from_table_with_filter( nhs_number, table_name, - attr_filter={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=delete_filter_expression, ) results.extend( diff --git a/lambdas/services/document_service.py b/lambdas/services/document_service.py index f3f82e882..34b511c90 100644 --- a/lambdas/services/document_service.py +++ b/lambdas/services/document_service.py @@ -1,5 +1,6 @@ from datetime import datetime, timezone +from boto3.dynamodb.conditions import Attr, ConditionBase from enums.metadata_field_names import DocumentReferenceMetadataFields from enums.s3_lifecycle_tags import S3LifecycleDays, S3LifecycleTags from enums.supported_document_types import SupportedDocumentTypes @@ -17,21 +18,23 @@ def __init__(self): self.dynamo_service = DynamoDBService() def fetch_available_document_references_by_type( - self, nhs_number: str, doc_type: SupportedDocumentTypes + self, + nhs_number: str, + doc_type: SupportedDocumentTypes, + query_filter: Attr | ConditionBase, ) -> list[DocumentReference]: results: list[DocumentReference] = [] - delete_filter = {DocumentReferenceMetadataFields.DELETED.value: ""} doc_type_table = doc_type.get_dynamodb_table_name() if isinstance(doc_type_table, list): for table in doc_type_table: results += self.fetch_documents_from_table_with_filter( - nhs_number, table, attr_filter=delete_filter + nhs_number, table, query_filter=query_filter ) return results return self.fetch_documents_from_table_with_filter( - nhs_number, doc_type_table, attr_filter=delete_filter + nhs_number, doc_type_table, query_filter=query_filter ) def fetch_documents_from_table( @@ -52,7 +55,7 @@ def fetch_documents_from_table( return documents def fetch_documents_from_table_with_filter( - self, nhs_number: str, table: str, attr_filter: dict + self, nhs_number: str, table: str, query_filter: Attr | ConditionBase ) -> list[DocumentReference]: documents = [] @@ -62,7 +65,7 @@ def fetch_documents_from_table_with_filter( search_key="NhsNumber", search_condition=nhs_number, requested_fields=DocumentReferenceMetadataFields.list(), - filtered_fields=attr_filter, + query_filter=query_filter, ) for item in response["Items"]: diff --git a/lambdas/tests/unit/enums/test_metadata_field_names.py b/lambdas/tests/unit/enums/test_metadata_field_names.py index 466cc1fa7..2ba7ddfc4 100755 --- a/lambdas/tests/unit/enums/test_metadata_field_names.py +++ b/lambdas/tests/unit/enums/test_metadata_field_names.py @@ -8,7 +8,7 @@ def test_can_get_one_field_name(): def test_returns_all_as_list(): subject = DocumentReferenceMetadataFields.list() - assert len(subject) == 14 + assert len(subject) == 12 assert DocumentReferenceMetadataFields.ID.value in subject assert DocumentReferenceMetadataFields.CONTENT_TYPE.value in subject assert DocumentReferenceMetadataFields.CREATED.value in subject @@ -16,8 +16,6 @@ def test_returns_all_as_list(): assert DocumentReferenceMetadataFields.FILE_NAME.value in subject assert DocumentReferenceMetadataFields.FILE_LOCATION.value in subject assert DocumentReferenceMetadataFields.NHS_NUMBER.value in subject - assert DocumentReferenceMetadataFields.TYPE.value in subject - assert DocumentReferenceMetadataFields.TTL.value in subject assert DocumentReferenceMetadataFields.VIRUS_SCANNER_RESULT.value in subject assert DocumentReferenceMetadataFields.CURRENT_GP_ODS.value in subject assert DocumentReferenceMetadataFields.UPLOADED.value in subject diff --git a/lambdas/tests/unit/helpers/data/dynamo_responses.py b/lambdas/tests/unit/helpers/data/dynamo_responses.py index 8b5a3964d..412722617 100755 --- a/lambdas/tests/unit/helpers/data/dynamo_responses.py +++ b/lambdas/tests/unit/helpers/data/dynamo_responses.py @@ -3,7 +3,7 @@ { "ID": "3d8683b9-1665-40d2-8499-6e8302d507ff", "ContentType": "type", - "Created": "2023-08-23T13:38:04.095Z", + "Created": "2024-01-01T12:00:00.000Z", "Deleted": "", "FileLocation": "s3://test-s3-bucket/9000000009/test-key-123", "FileName": "document.csv", @@ -12,12 +12,12 @@ "CurrentGpOds": "Y12345", "Uploaded": "True", "Uploading": "False", - "LastUpdated": "2023-08-23T13:38:04.095Z", + "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 }, { "ID": "4d8683b9-1665-40d2-8499-6e8302d507ff", "ContentType": "type", - "Created": "2023-08-23T13:38:04.095Z", + "Created": "2024-01-01T12:00:00.000Z", "Deleted": "", "FileLocation": "s3://test-s3-bucket/9000000009/test-key-223", "FileName": "results.pdf", @@ -26,12 +26,12 @@ "CurrentGpOds": "Y12345", "Uploaded": "True", "Uploading": "False", - "LastUpdated": "2023-08-23T13:38:04.095Z", + "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 }, { "ID": "5d8683b9-1665-40d2-8499-6e8302d507ff", "ContentType": "type", - "Created": "2023-08-24T14:38:04.095Z", + "Created": "2024-01-01T12:00:00.000Z", "Deleted": "", "FileLocation": "s3://test-s3-bucket/9000000009/test-key-323", "FileName": "output.csv", @@ -40,7 +40,7 @@ "CurrentGpOds": "Y12345", "Uploaded": "True", "Uploading": "False", - "LastUpdated": "2023-08-23T13:38:04.095Z", + "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 }, ], "Count": 3, diff --git a/lambdas/tests/unit/services/base/test_dynamo_service.py b/lambdas/tests/unit/services/base/test_dynamo_service.py index 799b71903..1cca255cc 100755 --- a/lambdas/tests/unit/services/base/test_dynamo_service.py +++ b/lambdas/tests/unit/services/base/test_dynamo_service.py @@ -1,10 +1,12 @@ import pytest -from boto3.dynamodb.conditions import Key +from boto3.dynamodb.conditions import Attr, Key from botocore.exceptions import ClientError +from enums.dynamo_filter import AttributeOperator from enums.metadata_field_names import DocumentReferenceMetadataFields from services.base.dynamo_service import DynamoDBService from tests.unit.conftest import MOCK_TABLE_NAME, TEST_NHS_NUMBER from tests.unit.helpers.data.dynamo_responses import MOCK_SEARCH_RESPONSE +from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder from utils.exceptions import DynamoServiceException MOCK_CLIENT_ERROR = ClientError( @@ -30,15 +32,22 @@ def mock_table(mocker, mock_service): yield mocker.patch.object(mock_service, "get_table") +@pytest.fixture +def mock_filter_expression(): + filter_builder = DynamoQueryFilterBuilder() + filter_expression = filter_builder.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator=AttributeOperator.EQUAL, + filter_value="", + ).build() + yield filter_expression + + def test_query_with_requested_fields_returns_items_from_dynamo( mock_service, mock_table ): search_key_obj = Key("NhsNumber").eq(TEST_NHS_NUMBER) - expected_projection = "#FileName_attr,#Created_attr" - expected_expr_attr_names = { - "#FileName_attr": "FileName", - "#Created_attr": "Created", - } + expected_projection = "FileName,Created" mock_table.return_value.query.return_value = MOCK_SEARCH_RESPONSE expected = MOCK_SEARCH_RESPONSE @@ -58,7 +67,6 @@ def test_query_with_requested_fields_returns_items_from_dynamo( mock_table.return_value.query.assert_called_once_with( IndexName="NhsNumberIndex", KeyConditionExpression=search_key_obj, - ExpressionAttributeNames=expected_expr_attr_names, ProjectionExpression=expected_projection, ) @@ -66,16 +74,11 @@ def test_query_with_requested_fields_returns_items_from_dynamo( def test_query_with_requested_fields_with_filter_returns_items_from_dynamo( - mock_service, mock_table + mock_service, mock_table, mock_filter_expression ): search_key_obj = Key("NhsNumber").eq(TEST_NHS_NUMBER) - expected_projection = "#FileName_attr,#Created_attr" - expected_expr_attr_names = { - "#FileName_attr": "FileName", - "#Created_attr": "Created", - } - expected_filter = "attribute_not_exists(Deleted) OR Deleted = :Deleted_val" - expected_attributes_values = {":Deleted_val": ""} + expected_projection = "FileName,Created" + expected_filter = Attr("Deleted").eq("") mock_table.return_value.query.return_value = MOCK_SEARCH_RESPONSE expected = MOCK_SEARCH_RESPONSE @@ -89,17 +92,15 @@ def test_query_with_requested_fields_with_filter_returns_items_from_dynamo( DocumentReferenceMetadataFields.FILE_NAME.value, DocumentReferenceMetadataFields.CREATED.value, ], - filtered_fields={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ) mock_table.assert_called_with(MOCK_TABLE_NAME) mock_table.return_value.query.assert_called_once_with( IndexName="NhsNumberIndex", KeyConditionExpression=search_key_obj, - ExpressionAttributeNames=expected_expr_attr_names, ProjectionExpression=expected_projection, FilterExpression=expected_filter, - ExpressionAttributeValues=expected_attributes_values, ) assert expected == actual diff --git a/lambdas/tests/unit/services/test_document_reference_search_service.py b/lambdas/tests/unit/services/test_document_reference_search_service.py index c1ed813a8..0fb5d7ff9 100644 --- a/lambdas/tests/unit/services/test_document_reference_search_service.py +++ b/lambdas/tests/unit/services/test_document_reference_search_service.py @@ -13,7 +13,7 @@ ] EXPECTED_RESPONSE = { - "created": "2023-08-23T13:38:04.095Z", + "created": "2024-01-01T12:00:00.000Z", "fileName": "document.csv", "virusScannerResult": "Clean", } diff --git a/lambdas/tests/unit/services/test_document_service.py b/lambdas/tests/unit/services/test_document_service.py index b12285d13..786e08f48 100644 --- a/lambdas/tests/unit/services/test_document_service.py +++ b/lambdas/tests/unit/services/test_document_service.py @@ -2,6 +2,7 @@ from unittest.mock import call import pytest +from enums.dynamo_filter import AttributeOperator from enums.metadata_field_names import DocumentReferenceMetadataFields from enums.s3_lifecycle_tags import S3LifecycleDays, S3LifecycleTags from enums.supported_document_types import SupportedDocumentTypes @@ -18,6 +19,7 @@ MOCK_EMPTY_RESPONSE, MOCK_SEARCH_RESPONSE, ) +from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder MOCK_DOCUMENT = MOCK_SEARCH_RESPONSE["Items"][0] @@ -44,13 +46,24 @@ def mock_s3_service(mocker, mock_service): yield mock_service.s3_service +@pytest.fixture +def mock_filter_expression(): + filter_builder = DynamoQueryFilterBuilder() + filter_expression = filter_builder.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator=AttributeOperator.EQUAL, + filter_value="", + ).build() + yield filter_expression + + def test_fetch_available_document_references_by_type_lg_returns_list_of_doc_references( - mock_service, mock_dynamo_service + mock_service, mock_dynamo_service, mock_filter_expression ): mock_dynamo_service.query_with_requested_fields.return_value = MOCK_SEARCH_RESPONSE results = mock_service.fetch_available_document_references_by_type( - TEST_NHS_NUMBER, SupportedDocumentTypes.LG + TEST_NHS_NUMBER, SupportedDocumentTypes.LG, mock_filter_expression ) assert len(results) == 3 @@ -63,17 +76,17 @@ def test_fetch_available_document_references_by_type_lg_returns_list_of_doc_refe search_key="NhsNumber", search_condition=TEST_NHS_NUMBER, requested_fields=DocumentReferenceMetadataFields.list(), - filtered_fields={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ) def test_fetch_available_document_references_by_type_arf_returns_list_of_doc_references( - mock_service, mock_dynamo_service + mock_service, mock_dynamo_service, mock_filter_expression ): mock_dynamo_service.query_with_requested_fields.return_value = MOCK_SEARCH_RESPONSE results = mock_service.fetch_available_document_references_by_type( - TEST_NHS_NUMBER, SupportedDocumentTypes.ARF + TEST_NHS_NUMBER, SupportedDocumentTypes.ARF, mock_filter_expression ) assert len(results) == 3 @@ -86,17 +99,17 @@ def test_fetch_available_document_references_by_type_arf_returns_list_of_doc_ref search_key="NhsNumber", search_condition=TEST_NHS_NUMBER, requested_fields=DocumentReferenceMetadataFields.list(), - filtered_fields={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ) def test_fetch_available_document_references_by_type_all_returns_list_of_doc_references( - mocker, mock_service, mock_dynamo_service + mocker, mock_service, mock_dynamo_service, mock_filter_expression ): mock_dynamo_service.query_with_requested_fields.return_value = MOCK_SEARCH_RESPONSE results = mock_service.fetch_available_document_references_by_type( - TEST_NHS_NUMBER, SupportedDocumentTypes.ALL + TEST_NHS_NUMBER, SupportedDocumentTypes.ALL, mock_filter_expression ) assert len(results) == 6 @@ -110,7 +123,7 @@ def test_fetch_available_document_references_by_type_all_returns_list_of_doc_ref search_key="NhsNumber", search_condition=TEST_NHS_NUMBER, requested_fields=DocumentReferenceMetadataFields.list(), - filtered_fields={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ), mocker.call( table_name=MOCK_LG_TABLE_NAME, @@ -118,7 +131,7 @@ def test_fetch_available_document_references_by_type_all_returns_list_of_doc_ref search_key="NhsNumber", search_condition=TEST_NHS_NUMBER, requested_fields=DocumentReferenceMetadataFields.list(), - filtered_fields={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ), ] @@ -128,7 +141,7 @@ def test_fetch_available_document_references_by_type_all_returns_list_of_doc_ref def test_fetch_available_document_references_by_type_all_only_one_result_is_returned_returns_doc_references( - mocker, mock_service, mock_dynamo_service + mocker, mock_service, mock_dynamo_service, mock_filter_expression ): mock_dynamo_service.query_with_requested_fields.side_effect = [ MOCK_SEARCH_RESPONSE, @@ -136,7 +149,7 @@ def test_fetch_available_document_references_by_type_all_only_one_result_is_retu ] results = mock_service.fetch_available_document_references_by_type( - TEST_NHS_NUMBER, SupportedDocumentTypes.ALL + TEST_NHS_NUMBER, SupportedDocumentTypes.ALL, mock_filter_expression ) assert len(results) == 3 @@ -150,7 +163,7 @@ def test_fetch_available_document_references_by_type_all_only_one_result_is_retu search_key="NhsNumber", search_condition=TEST_NHS_NUMBER, requested_fields=DocumentReferenceMetadataFields.list(), - filtered_fields={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ), mocker.call( table_name=MOCK_LG_TABLE_NAME, @@ -158,7 +171,7 @@ def test_fetch_available_document_references_by_type_all_only_one_result_is_retu search_key="NhsNumber", search_condition=TEST_NHS_NUMBER, requested_fields=DocumentReferenceMetadataFields.list(), - filtered_fields={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ), ] @@ -168,12 +181,12 @@ def test_fetch_available_document_references_by_type_all_only_one_result_is_retu def test_fetch_available_document_references_by_type_lg_returns_empty_list_of_doc_references( - mock_service, mock_dynamo_service + mock_service, mock_dynamo_service, mock_filter_expression ): mock_dynamo_service.query_with_requested_fields.return_value = MOCK_EMPTY_RESPONSE result = mock_service.fetch_available_document_references_by_type( - TEST_NHS_NUMBER, SupportedDocumentTypes.LG + TEST_NHS_NUMBER, SupportedDocumentTypes.LG, mock_filter_expression ) assert len(result) == 0 @@ -183,12 +196,12 @@ def test_fetch_available_document_references_by_type_lg_returns_empty_list_of_do search_key="NhsNumber", search_condition=TEST_NHS_NUMBER, requested_fields=DocumentReferenceMetadataFields.list(), - filtered_fields={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ) def test_fetch_documents_from_table_with_filter_returns_list_of_doc_references( - mocker, mock_service, mock_dynamo_service + mocker, mock_service, mock_dynamo_service, mock_filter_expression ): expected_calls = [ mocker.call( @@ -197,7 +210,7 @@ def test_fetch_documents_from_table_with_filter_returns_list_of_doc_references( search_key="NhsNumber", search_condition=TEST_NHS_NUMBER, requested_fields=DocumentReferenceMetadataFields.list(), - filtered_fields={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ) ] @@ -206,7 +219,7 @@ def test_fetch_documents_from_table_with_filter_returns_list_of_doc_references( results = mock_service.fetch_documents_from_table_with_filter( nhs_number=TEST_NHS_NUMBER, table=MOCK_LG_TABLE_NAME, - attr_filter={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ) assert len(results) == 3 @@ -219,7 +232,7 @@ def test_fetch_documents_from_table_with_filter_returns_list_of_doc_references( def test_fetch_documents_from_table_with_filter_returns_empty_list_of_doc_references( - mocker, mock_service, mock_dynamo_service + mocker, mock_service, mock_dynamo_service, mock_filter_expression ): expected_calls = [ mocker.call( @@ -228,7 +241,7 @@ def test_fetch_documents_from_table_with_filter_returns_empty_list_of_doc_refere search_key="NhsNumber", search_condition=TEST_NHS_NUMBER, requested_fields=DocumentReferenceMetadataFields.list(), - filtered_fields={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ) ] mock_dynamo_service.query_with_requested_fields.return_value = MOCK_EMPTY_RESPONSE @@ -236,7 +249,7 @@ def test_fetch_documents_from_table_with_filter_returns_empty_list_of_doc_refere results = mock_service.fetch_documents_from_table_with_filter( nhs_number=TEST_NHS_NUMBER, table=MOCK_LG_TABLE_NAME, - attr_filter={DocumentReferenceMetadataFields.DELETED.value: ""}, + query_filter=mock_filter_expression, ) assert len(results) == 0 diff --git a/lambdas/tests/unit/services/test_lloyd_george_stitch_service.py b/lambdas/tests/unit/services/test_lloyd_george_stitch_service.py index 0a1c6c8e0..c0ad0fdcd 100644 --- a/lambdas/tests/unit/services/test_lloyd_george_stitch_service.py +++ b/lambdas/tests/unit/services/test_lloyd_george_stitch_service.py @@ -37,7 +37,7 @@ def build_lg_doc_ref( "CurrentGpOds": "Y12345", "Uploaded": "True", "Uploading": "False", - "LastUpdated": "2023-08-23T13:38:04.095Z", + "LastUpdated": 1704110400, }, ) diff --git a/lambdas/tests/unit/utils/test_dynamo_query_filter_builder.py b/lambdas/tests/unit/utils/test_dynamo_query_filter_builder.py new file mode 100644 index 000000000..4f1c20f75 --- /dev/null +++ b/lambdas/tests/unit/utils/test_dynamo_query_filter_builder.py @@ -0,0 +1,158 @@ +import pytest +from boto3.dynamodb.conditions import Attr +from enums.dynamo_filter import AttributeOperator, ConditionOperator +from enums.metadata_field_names import DocumentReferenceMetadataFields +from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder +from utils.exceptions import DynamoServiceException + + +@pytest.fixture +def dynamo_filter(): + return DynamoQueryFilterBuilder() + + +def test_query_filter_builder_handles_single_equals_attribute_filter( + dynamo_filter, +): + expected = Attr("Deleted").eq("Test") + + actual = dynamo_filter.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator=AttributeOperator.EQUAL, + filter_value="Test", + ).build() + + assert actual == expected + + +def test_query_filter_builder_handles_single_not_equals_attribute_filter( + dynamo_filter, +): + expected = Attr("Deleted").ne("Test") + + actual = dynamo_filter.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator=AttributeOperator.NOT_EQUAL, + filter_value="Test", + ).build() + + assert actual == expected + + +def test_query_filter_builder_handles_single_less_than_attribute_filter( + dynamo_filter, +): + expected = Attr("Deleted").lt("Test") + + actual = dynamo_filter.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator=AttributeOperator.LESS_THAN, + filter_value="Test", + ).build() + + assert actual == expected + + +def test_query_filter_builder_handles_single_less_than_equal_to_attribute_filter( + dynamo_filter, +): + expected = Attr("Deleted").lte("Test") + + actual = dynamo_filter.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator=AttributeOperator.LESS_THAN_OR_EQUAL, + filter_value="Test", + ).build() + + assert actual == expected + + +def test_query_filter_builder_handles_single_greater_than_attribute_filter( + dynamo_filter, +): + expected = Attr("Deleted").gt("Test") + + actual = dynamo_filter.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator=AttributeOperator.GREATER_THAN, + filter_value="Test", + ).build() + + assert actual == expected + + +def test_query_filter_builder_handles_single_greater_than_equal_to_attribute_filter( + dynamo_filter, +): + expected = Attr("Deleted").gte("Test") + + actual = dynamo_filter.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator=AttributeOperator.GREATER_OR_EQUAL, + filter_value="Test", + ).build() + + assert actual == expected + + +def test_query_filter_builder_handles_multiple_attributes_with_and_operator( + dynamo_filter, +): + expected = Attr("Deleted").eq("Test") & Attr("FileName").eq("Test") + + actual = ( + dynamo_filter.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator=AttributeOperator.EQUAL, + filter_value="Test", + ) + .add_condition( + attribute=DocumentReferenceMetadataFields.FILE_NAME.value, + attr_operator=AttributeOperator.EQUAL, + filter_value="Test", + ) + .build() + ) + + assert actual == expected + + +def test_query_filter_builder_handles_multiple_attributes_with_or_operator( + dynamo_filter, +): + expected = Attr("Deleted").eq("Test") | Attr("FileName").eq("Test") + + actual = ( + dynamo_filter.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator=AttributeOperator.EQUAL, + filter_value="Test", + ) + .add_condition( + attribute=DocumentReferenceMetadataFields.FILE_NAME.value, + attr_operator=AttributeOperator.EQUAL, + filter_value="Test", + ) + .set_combination_operator(operator=ConditionOperator.OR) + .build() + ) + + assert actual == expected + + +def test_query_filter_builder_invalid_conditions_operator_raises_exception( + dynamo_filter, +): + with pytest.raises(DynamoServiceException): + dynamo_filter.add_condition( + attribute=str(DocumentReferenceMetadataFields.DELETED.value), + attr_operator="invalid", + filter_value="Test", + ) + + +def test_query_filter_builder_empty_conditions_build_raises_exception( + dynamo_filter, +): + with pytest.raises(DynamoServiceException): + dynamo_filter.build() diff --git a/lambdas/tests/unit/utils/test_dynamo_utils.py b/lambdas/tests/unit/utils/test_dynamo_utils.py index 01ff11620..4182e81aa 100644 --- a/lambdas/tests/unit/utils/test_dynamo_utils.py +++ b/lambdas/tests/unit/utils/test_dynamo_utils.py @@ -1,6 +1,5 @@ from enums.metadata_field_names import DocumentReferenceMetadataFields from utils.dynamo_utils import ( - create_attribute_filter, create_expression_attribute_placeholder, create_expression_attribute_values, create_expression_value_placeholder, @@ -22,17 +21,17 @@ def test_create_expressions_correctly_creates_an_expression_of_one_field(): def test_create_expressions_correctly_creates_an_expression_of_multiple_fields(): - expected_projection = "#NhsNumber_attr,#FileLocation_attr,#Type_attr" + expected_projection = "#NhsNumber_attr,#FileLocation_attr,#ContentType_attr" expected_expr_attr_names = { "#NhsNumber_attr": "NhsNumber", "#FileLocation_attr": "FileLocation", - "#Type_attr": "Type", + "#ContentType_attr": "ContentType", } fields_requested = [ DocumentReferenceMetadataFields.NHS_NUMBER.value, DocumentReferenceMetadataFields.FILE_LOCATION.value, - DocumentReferenceMetadataFields.TYPE.value, + DocumentReferenceMetadataFields.CONTENT_TYPE.value, ] actual_projection, actual_expr_attr_names = create_expressions(fields_requested) @@ -53,81 +52,6 @@ def test_create_expression_attribute_values(): assert actual == expected -def test_create_attr_filter_multiple_existing_values(): - fields_filter = { - DocumentReferenceMetadataFields.DELETED.value: "True", - DocumentReferenceMetadataFields.FILE_NAME.value: "Test Filename", - } - expected = "Deleted = :Deleted_val AND FileName = :FileName_val" - - actual = create_attribute_filter(fields_filter) - - assert actual == expected - - -def test_create_attr_filter_multiple_mixed_values(): - fields_filter = { - DocumentReferenceMetadataFields.DELETED.value: "", - DocumentReferenceMetadataFields.FILE_NAME.value: "Test Filename", - } - expected = ( - "attribute_not_exists(Deleted) OR Deleted = :Deleted_val AND " - "FileName = :FileName_val" - ) - - actual = create_attribute_filter(fields_filter) - - assert actual == expected - - -def test_create_attr_filter_multiple_empty_values(): - fields_filter = { - DocumentReferenceMetadataFields.DELETED.value: "", - DocumentReferenceMetadataFields.FILE_NAME.value: "", - } - expected = ( - "attribute_not_exists(Deleted) OR Deleted = :Deleted_val AND " - "attribute_not_exists(FileName) OR FileName = :FileName_val" - ) - - actual = create_attribute_filter(fields_filter) - - assert actual == expected - - -def test_create_attr_filter_multiple_none_values(): - fields_filter = { - DocumentReferenceMetadataFields.DELETED.value: None, - DocumentReferenceMetadataFields.FILE_NAME.value: None, - } - expected = ( - "attribute_not_exists(Deleted) OR Deleted = :Deleted_val AND " - "attribute_not_exists(FileName) OR FileName = :FileName_val" - ) - - actual = create_attribute_filter(fields_filter) - - assert actual == expected - - -def test_create_attr_filter_singular_existing_value(): - fields_filter = {DocumentReferenceMetadataFields.DELETED.value: "True"} - expected = "Deleted = :Deleted_val" - - actual = create_attribute_filter(fields_filter) - - assert actual == expected - - -def test_create_attr_filter_singular_empty_value(): - fields_filter = {DocumentReferenceMetadataFields.DELETED.value: ""} - expected = "attribute_not_exists(Deleted) OR Deleted = :Deleted_val" - - actual = create_attribute_filter(fields_filter) - - assert actual == expected - - def test_create_update_expression_multiple_values(): field_names = ["Deleted", "VirusScannerResult"] expected = "SET #Deleted_attr = :Deleted_val, #VirusScannerResult_attr = :VirusScannerResult_val" diff --git a/lambdas/utils/dynamo_query_filter_builder.py b/lambdas/utils/dynamo_query_filter_builder.py new file mode 100644 index 000000000..bdb35dae9 --- /dev/null +++ b/lambdas/utils/dynamo_query_filter_builder.py @@ -0,0 +1,51 @@ +from boto3.dynamodb.conditions import Attr, ConditionBase +from enums.dynamo_filter import AttributeOperator, ConditionOperator +from utils.exceptions import DynamoServiceException + + +class DynamoQueryFilterBuilder: + def __init__(self): + self.filter_conditions: list[Attr] = [] + self.conditions_operator: ConditionOperator = ConditionOperator.AND + + def add_condition( + self, attribute: str, attr_operator: AttributeOperator, filter_value + ): + match attr_operator: + case AttributeOperator.EQUAL: + condition = Attr(attribute).eq(filter_value) + case AttributeOperator.NOT_EQUAL: + condition = Attr(attribute).ne(filter_value) + case AttributeOperator.GREATER_THAN: + condition = Attr(attribute).gt(filter_value) + case AttributeOperator.GREATER_OR_EQUAL: + condition = Attr(attribute).gte(filter_value) + case AttributeOperator.LESS_THAN: + condition = Attr(attribute).lt(filter_value) + case AttributeOperator.LESS_THAN_OR_EQUAL: + condition = Attr(attribute).lte(filter_value) + case _: + raise DynamoServiceException( + f"Unsupported attribute filter operator: {attr_operator}" + ) + + self.filter_conditions.append(condition) + return self + + def set_combination_operator(self, operator: ConditionOperator): + self.conditions_operator = operator + return self + + def build(self) -> Attr | ConditionBase: + if not self.filter_conditions: + raise DynamoServiceException("Unable to build empty attribute filter") + + combined_condition = self.filter_conditions[0] + for condition in self.filter_conditions[1:]: + if self.conditions_operator == ConditionOperator.AND: + combined_condition &= condition + elif self.conditions_operator == ConditionOperator.OR: + combined_condition |= condition + + self.filter_conditions = [] + return combined_condition diff --git a/lambdas/utils/dynamo_utils.py b/lambdas/utils/dynamo_utils.py index 53e6602f7..0df5e3aff 100644 --- a/lambdas/utils/dynamo_utils.py +++ b/lambdas/utils/dynamo_utils.py @@ -30,44 +30,6 @@ def create_expressions(requested_fields: list) -> tuple[str, dict]: return projection_expression, expression_attribute_names -def create_attribute_filter(filtered_fields: dict) -> str: - """ - Creates a filter for dynamodb queries for existing and non-existing attributes - :param filtered_fields: Dictionary of filtered fields - - example usage: - fields_filter = { - DocumentReferenceMetadataFields.DELETED.value: "", - DocumentReferenceMetadataFields.FILENAME.value: "Test Filename" - } - attribute_filter = create_attribute_filter(fields_filter) - - result: - "attribute_not_exists(Deleted) OR Deleted = :Deleted_val AND " - "Filename = :Filename_val" - - """ - attr_filter = "" - - for field_name, field_value in filtered_fields.items(): - base_filter_string = ( - f"{field_name} = {create_expression_value_placeholder(field_name)}" - ) - if not field_value: - filter_string = ( - f"attribute_not_exists({field_name}) OR {base_filter_string}" - ) - else: - filter_string = base_filter_string - - if not attr_filter: - attr_filter = filter_string - else: - attr_filter += f" AND {filter_string}" - - return attr_filter - - def create_update_expression(field_names: list) -> str: """ Creates an expression for dynamodb queries to SET a new value for an item