diff --git a/lambdas/handlers/generate_document_manifest_handler.py b/lambdas/handlers/generate_document_manifest_handler.py index d9e1b3778..9aa6a3e12 100644 --- a/lambdas/handlers/generate_document_manifest_handler.py +++ b/lambdas/handlers/generate_document_manifest_handler.py @@ -1,4 +1,3 @@ -from boto3.dynamodb.types import TypeDeserializer from enums.lambda_error import LambdaError from enums.logging_app_interaction import LoggingAppInteraction from models.zip_trace import DocumentManifestZipTrace @@ -9,6 +8,7 @@ from utils.decorators.handle_lambda_exceptions import handle_lambda_exceptions from utils.decorators.override_error_check import override_error_check from utils.decorators.set_audit_arg import set_request_context_for_logging +from utils.dynamo_utils import deserialize_dynamodb_object from utils.lambda_exceptions import GenerateManifestZipException from utils.lambda_response import ApiGatewayResponse from utils.request_context import request_context @@ -71,9 +71,4 @@ def manifest_zip_handler(zip_trace_item): def prepare_zip_trace_data(new_zip_trace: dict) -> dict: - deserialize = TypeDeserializer().deserialize - parsed_dynamodb_items = { - key: deserialize(dynamodb_value) - for key, dynamodb_value in new_zip_trace.items() - } - return parsed_dynamodb_items + return deserialize_dynamodb_object(new_zip_trace) diff --git a/lambdas/services/base/dynamo_service.py b/lambdas/services/base/dynamo_service.py index ec8de0b90..7f8584220 100644 --- a/lambdas/services/base/dynamo_service.py +++ b/lambdas/services/base/dynamo_service.py @@ -11,6 +11,8 @@ create_expression_attribute_values, create_expressions, create_update_expression, + deserialize_dynamodb_object, + serialize_dict_to_dynamodb_object, ) from utils.exceptions import DynamoServiceException @@ -29,6 +31,7 @@ def __new__(cls): def __init__(self): if not self.initialised: self.dynamodb = boto3.resource("dynamodb", region_name="eu-west-2") + self.client = boto3.client("dynamodb") self.initialised = True def get_table(self, table_name: str): @@ -196,7 +199,7 @@ def update_item( table_name: str, key_pair: dict[str, str], updated_fields: dict, - condition_expression: str | ConditionBase| None = None, + condition_expression: str | ConditionBase | None = None, expression_attribute_values: dict | None = None, ): table = self.get_table(table_name) @@ -429,3 +432,57 @@ def build_update_transaction_item( }, } } + + def query_table_with_paginator( + self, + table_name: str, + index_name: str, + key: str, + condition: str, + filter_expression: str | None = None, + expression_attribute_names: str | None = None, + expression_attribute_values: dict | None = None, + limit: int = 20, + page_size: int = 1, + start_key: str | None = None, + ) -> dict: + + try: + query_params = { + "TableName": table_name, + "IndexName": index_name, + "KeyConditionExpression": f"{key}=:i", + "PaginationConfig": { + "MaxItems": limit, + "PageSize": page_size, + "StartingToken": start_key, + }, + } + + if expression_attribute_values is None: + expression_attribute_values = {} + + expression_attribute_values[":i"] = condition + + if filter_expression: + query_params["FilterExpression"] = filter_expression + + if expression_attribute_names: + query_params["ExpressionAttributeNames"] = expression_attribute_names + + if expression_attribute_values: + query_params["ExpressionAttributeValues"] = ( + serialize_dict_to_dynamodb_object(expression_attribute_values) + ) + + paginator = self.client.get_paginator("query") + response = paginator.paginate(**query_params).build_full_result() + + response["Items"] = [ + deserialize_dynamodb_object(item) for item in response["Items"] + ] + return response + + except Exception as e: + logger.error("Failed to query DynamoDB") + raise e diff --git a/lambdas/services/document_service.py b/lambdas/services/document_service.py index 1996fc45c..7e5fabd60 100644 --- a/lambdas/services/document_service.py +++ b/lambdas/services/document_service.py @@ -294,3 +294,45 @@ def create_dynamo_entry( except (ValidationError, ClientError) as e: logger.error(e) raise e + + def query_table_with_paginator( + self, + index_name: str, + search_key: str, + search_condition: str, + table_name: str | None = None, + filter_expression: str | None = None, + expression_attribute_names: dict | None = None, + expression_attribute_values: dict | None = None, + limit: int | None = None, + page_size: int = 1, + start_key: str | None = None, + model_class: BaseModel | None = None, + ) -> tuple[list[BaseModel], str | None]: + + try: + table_name = table_name or self.table_name + model_class = model_class or self.model_class + + response = self.dynamo_service.query_table_with_paginator( + table_name=table_name, + index_name=index_name, + key=search_key, + condition=search_condition, + filter_expression=filter_expression, + expression_attribute_names=expression_attribute_names, + expression_attribute_values=expression_attribute_values, + limit=limit, + page_size=page_size, + start_key=start_key, + ) + + references = [ + model_class.model_validate(item) for item in response["Items"] + ] + + return references, response.get("NextToken") + + except (ValidationError, ClientError) as e: + logger.error(e) + raise e diff --git a/lambdas/services/document_upload_review_service.py b/lambdas/services/document_upload_review_service.py index 9fefa670c..cbbd5478e 100644 --- a/lambdas/services/document_upload_review_service.py +++ b/lambdas/services/document_upload_review_service.py @@ -13,7 +13,7 @@ from utils.audit_logging_setup import LoggingService from utils.aws_transient_error_check import is_transient_error from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder -from utils.dynamo_utils import build_transaction_item +from utils.dynamo_utils import build_mixed_condition_expression, build_transaction_item from utils.exceptions import DocumentReviewException logger = LoggingService(__name__) @@ -41,35 +41,34 @@ def model_class(self) -> type: def s3_bucket(self) -> str: return self._s3_bucket - def query_docs_pending_review_by_custodian_with_limit( + def query_docs_pending_review_with_paginator( self, ods_code: str, limit: int = DEFAULT_QUERY_LIMIT, - start_key: dict | None = None, + start_key: str | None = None, nhs_number: str | None = None, uploader: str | None = None, - ) -> tuple[list[DocumentUploadReviewReference], dict | None]: - logger.info(f"Getting review document references for custodian: {ods_code}") - - filter_expression = self.build_review_dynamo_filter( - nhs_number=nhs_number, uploader=uploader - ) + ) -> tuple[list[DocumentUploadReviewReference], str | None]: try: - response = self.dynamo_service.query_table_single( - table_name=self.table_name, + logger.info(f"Getting review document references for custodian: {ods_code}") + + filter_expression, condition_attribute_names, condition_attribute_values = ( + self.build_paginator_query_filter( + nhs_number=nhs_number, uploader=uploader + ) + ) + references, last_evaluated_key = self.query_table_with_paginator( + index_name="CustodianIndex", search_key="Custodian", search_condition=ods_code, - index_name="CustodianIndex", + filter_expression=filter_expression, + expression_attribute_names=condition_attribute_names, + expression_attribute_values=condition_attribute_values, limit=limit, start_key=start_key, - query_filter=filter_expression, ) - references = self._validate_review_references(response["Items"]) - - last_evaluated_key = response.get("LastEvaluatedKey", None) - return references, last_evaluated_key except ClientError as e: @@ -89,6 +88,36 @@ def _validate_review_references( logger.error(e) raise DocumentReviewException(ErrorMessage.FAILED_TO_VALIDATE.value) + def build_paginator_query_filter( + self, nhs_number: str | None = None, uploader: str | None = None + ): + conditions = [ + { + "field": "ReviewStatus", + "operator": "=", + "value": DocumentReviewStatus.PENDING_REVIEW.value, + } + ] + if nhs_number: + conditions.append( + { + "field": "NhsNumber", + "operator": "=", + "value": nhs_number, + } + ) + + if uploader: + conditions.append( + { + "field": "Author", + "operator": "=", + "value": uploader, + } + ) + + return build_mixed_condition_expression(conditions) + def get_document( self, document_id: str, version: int | None ) -> DocumentUploadReviewReference | None: diff --git a/lambdas/services/search_document_review_service.py b/lambdas/services/search_document_review_service.py index 0d916944f..83a03afab 100644 --- a/lambdas/services/search_document_review_service.py +++ b/lambdas/services/search_document_review_service.py @@ -1,7 +1,3 @@ -import base64 -import decimal -import json - from enums.lambda_error import LambdaError from pydantic import ValidationError from services.document_upload_review_service import DocumentUploadReviewService @@ -20,14 +16,13 @@ def process_request( self, ods_code: str, params: dict ) -> tuple[list[str], str | None]: try: - - decoded_start_key = self.decode_start_key(params.get("nextPageToken", None)) + start_key = params.get("nextPageToken", None) str_limit = params.get("limit", self.document_service.DEFAULT_QUERY_LIMIT) limit = int(str_limit) references, last_evaluated_key = self.get_review_document_references( - start_key=decoded_start_key, + start_key=start_key, ods_code=ods_code, limit=limit, nhs_number=params.get("nhsNumber", None), @@ -50,9 +45,7 @@ def process_request( for reference in references ] - encoded_exclusive_start_key = self.encode_start_key(last_evaluated_key) - - return output_refs, encoded_exclusive_start_key + return output_refs, last_evaluated_key except ValidationError as e: logger.error(e) @@ -69,33 +62,14 @@ def get_review_document_references( self, ods_code: str, limit: int | None = None, - start_key: dict | None = None, + start_key: str | None = None, nhs_number: str | None = None, uploader: str | None = None, ): - return self.document_service.query_docs_pending_review_by_custodian_with_limit( + return self.document_service.query_docs_pending_review_with_paginator( ods_code=ods_code, limit=limit, start_key=start_key, nhs_number=nhs_number, uploader=uploader, ) - - def decode_start_key(self, encoded_start_key: str | None) -> dict[str, str] | None: - return ( - json.loads( - base64.b64decode(encoded_start_key.encode("ascii")).decode("utf-8") - ) - if encoded_start_key - else None - ) - - def encode_start_key(self, start_key: dict) -> str | None: - if start_key: - for key, value in start_key.items(): - if isinstance(value, decimal.Decimal): - start_key[key] = int(value) - return base64.b64encode(json.dumps(start_key).encode("ascii")).decode( - "utf-8" - ) - return None diff --git a/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py b/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py index 72f464152..ddb99b5a8 100644 --- a/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py +++ b/lambdas/tests/unit/helpers/data/search_document_review/dynamo_response.py @@ -30,7 +30,7 @@ "UploadDate": 1704110400, "NhsNumber": TEST_NHS_NUMBER, "ReviewReason": DocumentReviewReason.FILE_COUNT_MISMATCH, - "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW, + "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW.value, "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 "DocumentSnomedCodeType": SnomedCodes.LLOYD_GEORGE.value.code, }, @@ -52,7 +52,7 @@ "UploadDate": 1704110400, "NhsNumber": TEST_NHS_NUMBER, "ReviewReason": DocumentReviewReason.FILE_COUNT_MISMATCH, - "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW, + "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW.value, "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 "DocumentSnomedCodeType": SnomedCodes.LLOYD_GEORGE.value.code, }, @@ -72,11 +72,9 @@ "Author": MOCK_PREVIOUS_ODS_CODE, "Custodian": TEST_CURRENT_GP_ODS, "UploadDate": 1704110400, - "Reviewer": None, "NhsNumber": TEST_NHS_NUMBER, - "ReviewDate": None, "ReviewReason": DocumentReviewReason.FILE_COUNT_MISMATCH, - "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW, + "ReviewStatus": DocumentReviewStatus.PENDING_REVIEW.value, "LastUpdated": 1704110400, # Timestamp: 2024-01-01T12:00:00 "DocumentSnomedCodeType": SnomedCodes.LLOYD_GEORGE.value.code, }, @@ -99,3 +97,105 @@ }, "LastEvaluatedKey": TEST_UUID, } + +MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE = { + "Items": [ + { + "ID": {"S": "3d8683b9-1665-40d2-8499-6e8302d507ff"}, + "Version": {"N": "1"}, + "Files": { + "L": [ + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123" + }, + "FileName": {"S": "document.csv"}, + } + }, + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223" + }, + "FileName": {"S": "results.pdf"}, + } + }, + ] + }, + "Author": {"S": MOCK_PREVIOUS_ODS_CODE}, + "Custodian": {"S": TEST_CURRENT_GP_ODS}, + "UploadDate": {"N": "1704110400"}, + "NhsNumber": {"S": TEST_NHS_NUMBER}, + "ReviewReason": {"S": DocumentReviewReason.FILE_COUNT_MISMATCH}, + "ReviewStatus": {"S": DocumentReviewStatus.PENDING_REVIEW.value}, + "LastUpdated": {"N": "1704110400"}, + "DocumentSnomedCodeType": {"S": SnomedCodes.LLOYD_GEORGE.value.code}, + }, + { + "ID": {"S": "4d8683b9-1665-40d2-8499-6e8302d507ff"}, + "Version": {"N": "2"}, + "Files": { + "L": [ + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123" + }, + "FileName": {"S": "document.csv"}, + } + }, + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223" + }, + "FileName": {"S": "results.pdf"}, + } + }, + ] + }, + "Author": {"S": MOCK_PREVIOUS_ODS_CODE}, + "Custodian": {"S": TEST_CURRENT_GP_ODS}, + "UploadDate": {"N": "1704110400"}, + "NhsNumber": {"S": TEST_NHS_NUMBER}, + "ReviewReason": {"S": DocumentReviewReason.FILE_COUNT_MISMATCH}, + "ReviewStatus": {"S": DocumentReviewStatus.PENDING_REVIEW.value}, + "LastUpdated": {"N": "1704110400"}, + "DocumentSnomedCodeType": {"S": SnomedCodes.LLOYD_GEORGE.value.code}, + }, + { + "ID": {"S": "5d8683b9-1665-40d2-8499-6e8302d507ff"}, + "Version": {"N": "3"}, + "Files": { + "L": [ + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-123" + }, + "FileName": {"S": "document.csv"}, + } + }, + { + "M": { + "FileLocation": { + "S": f"s3://{MOCK_STAGING_STORE_BUCKET}/{TEST_NHS_NUMBER}/test-key-223" + }, + "FileName": {"S": "results.pdf"}, + } + }, + ] + }, + "Author": {"S": MOCK_PREVIOUS_ODS_CODE}, + "Custodian": {"S": TEST_CURRENT_GP_ODS}, + "UploadDate": {"N": "1704110400"}, + "NhsNumber": {"S": TEST_NHS_NUMBER}, + "ReviewReason": {"S": DocumentReviewReason.FILE_COUNT_MISMATCH}, + "ReviewStatus": {"S": DocumentReviewStatus.PENDING_REVIEW.value}, + "LastUpdated": {"N": "1704110400"}, + "DocumentSnomedCodeType": {"S": SnomedCodes.LLOYD_GEORGE.value.code}, + }, + ], + "NextToken": TEST_UUID, +} diff --git a/lambdas/tests/unit/services/base/test_dynamo_service.py b/lambdas/tests/unit/services/base/test_dynamo_service.py index 80fa1ea91..6d66c6673 100755 --- a/lambdas/tests/unit/services/base/test_dynamo_service.py +++ b/lambdas/tests/unit/services/base/test_dynamo_service.py @@ -1,8 +1,8 @@ import copy -from unittest.mock import call +from unittest.mock import MagicMock, call import pytest -from boto3.dynamodb.conditions import Attr, Equals, Key, And +from boto3.dynamodb.conditions import And, Attr, Equals, Key from botocore.exceptions import ClientError from enums.dynamo_filter import AttributeOperator from enums.metadata_field_names import DocumentReferenceMetadataFields @@ -17,14 +17,24 @@ MOCK_RESPONSE, MOCK_RESPONSE_WITH_LAST_KEY, ) +from tests.unit.helpers.data.search_document_review.dynamo_response import ( + MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE, + MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE, +) from utils.dynamo_query_filter_builder import DynamoQueryFilterBuilder +from utils.dynamo_utils import ( + build_mixed_condition_expression, + serialize_dict_to_dynamodb_object, +) from utils.exceptions import DynamoServiceException @pytest.fixture def mock_service(set_env, mocker): mocker.patch("boto3.resource") + mock_client = MagicMock() service = DynamoDBService() + mocker.patch.object(service, "client", return_value=mock_client) yield service DynamoDBService.instance = None @@ -1196,3 +1206,80 @@ def test_build_key_condition_non_matching_list_lengths( mock_service.build_key_condition( search_key=search_key, search_condition=search_condition ) + + +def test_query_table_using_paginator(mock_service): + mock_paginator = mock_service.client.get_paginator.return_value = MagicMock() + + mock_paginator.paginate.return_value.build_full_result.return_value = ( + MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE + ) + + expected = { + "Items": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"], + "NextToken": MOCK_DOCUMENT_REVIEW_PAGINATOR_RESPONSE["NextToken"], + } + + actual = mock_service.query_table_with_paginator( + table_name=MOCK_TABLE_NAME, + index_name="NhsNumberIndex", + key="NhsNumber", + condition=TEST_NHS_NUMBER, + ) + + mock_paginator.paginate.assert_called_with( + TableName=MOCK_TABLE_NAME, + IndexName="NhsNumberIndex", + KeyConditionExpression="NhsNumber=:i", + ExpressionAttributeValues={":i": {"S": TEST_NHS_NUMBER}}, + PaginationConfig={"MaxItems": 20, "PageSize": 1, "StartingToken": None}, + ) + + assert actual == expected + + +def test_query_table_using_pagination_with_filter_expression(mock_service): + mock_paginator = mock_service.client.get_paginator.return_value = MagicMock() + + conditions = [ + { + "field": "ReviewStatus", + "operator": "=", + "value": "PENDING_REVIEW", + }, + { + "field": "NhsNumber", + "operator": "=", + "value": TEST_NHS_NUMBER, + }, + ] + filter_expression, condition_attribute_names, condition_attribute_values = ( + build_mixed_condition_expression(conditions=conditions) + ) + + serialized_condition_attribute_values = serialize_dict_to_dynamodb_object( + condition_attribute_values + ) + + mock_service.query_table_with_paginator( + table_name=MOCK_TABLE_NAME, + index_name="NhsNumberIndex", + key="NhsNumber", + condition=TEST_NHS_NUMBER, + filter_expression=filter_expression, + expression_attribute_names=condition_attribute_names, + expression_attribute_values=condition_attribute_values, + ) + + mock_paginator.paginate.assert_called_with( + TableName=MOCK_TABLE_NAME, + IndexName="NhsNumberIndex", + KeyConditionExpression="NhsNumber=:i", + FilterExpression=filter_expression, + ExpressionAttributeValues={ + ":i": {"S": TEST_NHS_NUMBER}, + **serialized_condition_attribute_values, + }, + ExpressionAttributeNames=condition_attribute_names, + PaginationConfig={"MaxItems": 20, "PageSize": 1, "StartingToken": None}, + ) diff --git a/lambdas/tests/unit/services/test_document_upload_review_service.py b/lambdas/tests/unit/services/test_document_upload_review_service.py index bde3cf742..ceefbfcee 100644 --- a/lambdas/tests/unit/services/test_document_upload_review_service.py +++ b/lambdas/tests/unit/services/test_document_upload_review_service.py @@ -15,7 +15,6 @@ from tests.unit.conftest import ( MOCK_DOCUMENT_REVIEW_BUCKET, MOCK_DOCUMENT_REVIEW_TABLE, - TEST_CURRENT_GP_ODS, TEST_NHS_NUMBER, TEST_UUID, ) @@ -536,227 +535,6 @@ def test_build_filter_handles_both_nhs_number_and_uploader(mock_service): assert actual == expected -def test_query_review_documents_queries_dynamodb_with_filter_expression_nhs_number_passed( - mock_service, mocker -): - mock_nhs_number_filter_builder = mocker.patch.object( - mock_service, "build_review_dynamo_filter" - ) - mock_nhs_number_filter_builder.return_value = Attr("NhsNumber").eq(TEST_NHS_NUMBER) - - mock_service.query_docs_pending_review_by_custodian_with_limit( - ods_code=TEST_ODS_CODE, nhs_number=TEST_NHS_NUMBER - ) - - mock_nhs_number_filter_builder.assert_called_with( - nhs_number=TEST_NHS_NUMBER, uploader=None - ) - mock_service.dynamo_service.query_table_single.assert_called_with( - table_name=MOCK_DOCUMENT_REVIEW_TABLE, - search_key="Custodian", - search_condition=TEST_ODS_CODE, - index_name="CustodianIndex", - limit=TEST_QUERY_LIMIT, - start_key=None, - query_filter=mock_nhs_number_filter_builder.return_value, - ) - - -def test_query_review_documents_queries_dynamodb_with_filter_expression_uploader_passed( - mock_service, mocker -): - mock_uploader_filter_builder = mocker.patch.object( - mock_service, "build_review_dynamo_filter" - ) - mock_uploader_filter_builder.return_value = Attr("Author").eq(NEW_ODS_CODE) - mock_service.query_docs_pending_review_by_custodian_with_limit( - ods_code=TEST_ODS_CODE, uploader=NEW_ODS_CODE - ) - mock_uploader_filter_builder.assert_called_with( - nhs_number=None, uploader=NEW_ODS_CODE - ) - mock_service.dynamo_service.query_table_single.assert_called_with( - table_name=MOCK_DOCUMENT_REVIEW_TABLE, - search_key="Custodian", - search_condition=TEST_ODS_CODE, - index_name="CustodianIndex", - limit=TEST_QUERY_LIMIT, - start_key=None, - query_filter=mock_uploader_filter_builder.return_value, - ) - - -def test_query_review_documents_by_custodian_handles_filtering_by_nhs_number_and_uploader( - mock_service, mocker -): - mock_uploader_filter_builder = mocker.patch.object( - mock_service, "build_review_dynamo_filter" - ) - mock_uploader_filter_builder.return_value = Attr("Author").eq(NEW_ODS_CODE) & Attr( - "NhsNumber" - ).eq(TEST_NHS_NUMBER) - mock_service.query_docs_pending_review_by_custodian_with_limit( - ods_code=TEST_ODS_CODE, uploader=NEW_ODS_CODE, nhs_number=TEST_NHS_NUMBER - ) - mock_uploader_filter_builder.assert_called_with( - nhs_number=TEST_NHS_NUMBER, uploader=NEW_ODS_CODE - ) - mock_service.dynamo_service.query_table_single.assert_called_with( - table_name=MOCK_DOCUMENT_REVIEW_TABLE, - search_key="Custodian", - search_condition=TEST_ODS_CODE, - index_name="CustodianIndex", - limit=TEST_QUERY_LIMIT, - start_key=None, - query_filter=mock_uploader_filter_builder.return_value, - ) - - -def test_update_document_review_status_success(mock_service, mocker): - mock_update_document = mocker.patch.object(mock_service, "update_document") - - review_document = MagicMock(spec=DocumentUploadReviewReference) - review_document.id = "test-review-id" - review_document.version = 1 - - mock_service.update_document_review_status(review_document) - mock_update_document.assert_called_once_with( - document=review_document, - key_pair={"ID": review_document.id, "Version": review_document.version}, - update_fields_name={"review_status", "files"}, - condition_expression=None, - ) - - -def test_update_document_review_status_success_with_condition(mock_service, mocker): - mock_update_document = mocker.patch.object(mock_service, "update_document") - - review_document = MagicMock(spec=DocumentUploadReviewReference) - review_document.id = "test-review-id" - review_document.version = 1 - condition = Attr("ReviewStatus").eq("PENDING_REVIEW") - mock_service.update_document_review_status(review_document, condition) - mock_update_document.assert_called_once_with( - document=review_document, - key_pair={"ID": review_document.id, "Version": review_document.version}, - update_fields_name={"review_status", "files"}, - condition_expression=condition, - ) - - -def test_update_document_review_status_error_handling_transient(mock_service, mocker): - mock_update_document = mocker.patch.object(mock_service, "update_document") - mock_logger = mocker.patch("services.document_upload_review_service.logger") - mock_is_transient = mocker.patch( - "services.document_upload_review_service.is_transient_error" - ) - - review_document = MagicMock(spec=DocumentUploadReviewReference) - review_document.id = "test-review-id" - review_document.version = 1 - mock_is_transient.return_value = True - transient_error = ClientError( - { - "Error": { - "Code": "InternalServerError", - "Message": "Internal server error", - }, - "ResponseMetadata": {"HTTPStatusCode": 500}, - }, - "UpdateItem", - ) - mock_update_document.side_effect = transient_error - - with pytest.raises(ClientError) as exc_info: - mock_service.update_document_review_status(review_document) - assert exc_info.value == transient_error - mock_logger.error.assert_called_once_with(transient_error) - - -def test_update_document_review_status_error_handling(mock_service, mocker): - mock_update_document = mocker.patch.object(mock_service, "update_document") - mock_logger = mocker.patch("services.document_upload_review_service.logger") - mock_is_transient = mocker.patch( - "services.document_upload_review_service.is_transient_error" - ) - - review_document = MagicMock(spec=DocumentUploadReviewReference) - review_document.id = "test-review-id" - review_document.version = 1 - mock_is_transient.return_value = False - non_transient_error = ClientError( - { - "Error": {"Code": "ValidationException", "Message": "Validation error"}, - "ResponseMetadata": {"HTTPStatusCode": 400}, - }, - "UpdateItem", - ) - mock_update_document.side_effect = non_transient_error - - with pytest.raises(DocumentReviewException) as exc_info: - mock_service.update_document_review_status(review_document) - assert str(exc_info.value) == "Error updating document review status" - mock_logger.error.assert_called_once_with(non_transient_error) - - -@freeze_time("2023-10-30T10:25:00") -def test_create_dynamo_entry_creates_review_document_reference_in_dynamodb_valid_reference( - mock_service, -): - valid_review_document_reference = DocumentUploadReviewReference( - id=TEST_UUID, - author=TEST_CURRENT_GP_ODS, - custodian=TEST_CURRENT_GP_ODS, - files=[ - DocumentReviewFileDetails(file_name="test_file.pdf", file_location="here") - ], - nhs_number=TEST_NHS_NUMBER, - ) - - mock_service.create_dynamo_entry(valid_review_document_reference) - - mock_service.dynamo_service.create_item.assert_called_with( - table_name=MOCK_DOCUMENT_REVIEW_TABLE, - item=valid_review_document_reference.model_dump( - by_alias=True, exclude_none=True - ), - ) - - -def test_create_dynamo_entry_throws_error_invalid_object_to_write_to_dynamodb( - mock_service, -): - invalid_entry = { - "author": TEST_CURRENT_GP_ODS, - "other_key": "this model has missing data", - } - - with pytest.raises(ValidationError): - mock_service.create_dynamo_entry(invalid_entry) - - mock_service.dynamo_service.create_item.assert_not_called() - - -def test_create_dynamo_entry_throws_error_dynamodb_error( - mock_service, -): - mock_service.dynamo_service.create_item.side_effect = ClientError( - {"error": "test error message"}, "test" - ) - valid_review_document_reference = DocumentUploadReviewReference( - id=TEST_UUID, - author=TEST_CURRENT_GP_ODS, - custodian=TEST_CURRENT_GP_ODS, - files=[ - DocumentReviewFileDetails(file_name="test_file.pdf", file_location="here") - ], - nhs_number=TEST_NHS_NUMBER, - ) - - with pytest.raises(ClientError): - mock_service.create_dynamo_entry(valid_review_document_reference) - - def test_get_document_returns_review_document(mock_service): mock_service.dynamo_service.get_item.return_value = { "Item": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][0] @@ -790,3 +568,105 @@ def test_get_document_by_id_raises_exception_client_error(mock_service): with pytest.raises(DocumentReviewException): mock_service.get_document(TEST_UUID, 1) + + +@pytest.mark.parametrize( + "nhs_number, uploader, expected", + [ + ( + None, + None, + ( + "#ReviewStatus_attr = :ReviewStatus_condition_val", + {"#ReviewStatus_attr": "ReviewStatus"}, + {":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW}, + ), + ), + ( + TEST_NHS_NUMBER, + None, + ( + "#ReviewStatus_attr = :ReviewStatus_condition_val AND #NhsNumber_attr = :NhsNumber_condition_val", + {"#ReviewStatus_attr": "ReviewStatus", "#NhsNumber_attr": "NhsNumber"}, + { + ":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, + ":NhsNumber_condition_val": TEST_NHS_NUMBER, + }, + ), + ), + ( + TEST_NHS_NUMBER, + TEST_ODS_CODE, + ( + ( + "#ReviewStatus_attr = :ReviewStatus_condition_val AND #NhsNumber_attr = :NhsNumber_condition_val AND " + "#Author_attr = :Author_condition_val" + ), + { + "#ReviewStatus_attr": "ReviewStatus", + "#NhsNumber_attr": "NhsNumber", + "#Author_attr": "Author", + }, + { + ":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, + ":NhsNumber_condition_val": TEST_NHS_NUMBER, + ":Author_condition_val": TEST_ODS_CODE, + }, + ), + ), + ( + None, + TEST_ODS_CODE, + ( + "#ReviewStatus_attr = :ReviewStatus_condition_val AND #Author_attr = :Author_condition_val", + {"#ReviewStatus_attr": "ReviewStatus", "#Author_attr": "Author"}, + { + ":ReviewStatus_condition_val": DocumentReviewStatus.PENDING_REVIEW, + ":Author_condition_val": TEST_ODS_CODE, + }, + ), + ), + ], +) +def test_build_paginator_query_filter(mock_service, nhs_number, uploader, expected): + actual = mock_service.build_paginator_query_filter( + nhs_number=nhs_number, uploader=uploader + ) + + assert actual == expected + + +def test_query_docs_pending_review_with_paginator(mock_service): + filter_expression, condition_attribute_names, condition_attribute_values = ( + mock_service.build_paginator_query_filter() + ) + + mock_service.dynamo_service.query_table_with_paginator.return_value = { + "Items": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"], + "NextToken": TEST_UUID, + } + + expected = ( + [ + DocumentUploadReviewReference(**item) + for item in MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"] + ], + TEST_UUID, + ) + + actual = mock_service.query_docs_pending_review_with_paginator(TEST_ODS_CODE) + + mock_service.dynamo_service.query_table_with_paginator.assert_called_with( + table_name=MOCK_DOCUMENT_REVIEW_TABLE, + index_name="CustodianIndex", + key="Custodian", + condition=TEST_ODS_CODE, + filter_expression=filter_expression, + expression_attribute_names=condition_attribute_names, + expression_attribute_values=condition_attribute_values, + limit=50, + start_key=None, + page_size=1, + ) + + assert actual == expected diff --git a/lambdas/tests/unit/services/test_search_document_review_service.py b/lambdas/tests/unit/services/test_search_document_review_service.py index 12341421e..9e747334d 100644 --- a/lambdas/tests/unit/services/test_search_document_review_service.py +++ b/lambdas/tests/unit/services/test_search_document_review_service.py @@ -40,10 +40,6 @@ def search_document_review_service(set_env, mocker): def test_handle_gateway_api_request_happy_path(search_document_review_service, mocker): - mocker.patch.object( - search_document_review_service, "decode_start_key" - ).return_value = TEST_LAST_EVALUATED_KEY - expected_refs = [ DocumentUploadReviewReference.model_validate(item) for item in MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"] @@ -52,7 +48,7 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m search_document_review_service, "get_review_document_references" ).return_value = ( expected_refs, - TEST_LAST_EVALUATED_KEY, + TEST_ENCODED_START_KEY, ) expected = ( @@ -66,9 +62,9 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m "nhsNumber": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][0][ "NhsNumber" ], - "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE[ - "Items" - ][0]["DocumentSnomedCodeType"], + "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][ + 0 + ]["DocumentSnomedCodeType"], "author": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][0]["Author"], "uploadDate": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][0][ "UploadDate" @@ -83,9 +79,9 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m "nhsNumber": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][1][ "NhsNumber" ], - "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE[ - "Items" - ][1]["DocumentSnomedCodeType"], + "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][ + 1 + ]["DocumentSnomedCodeType"], "author": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][1]["Author"], "uploadDate": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][1][ "UploadDate" @@ -100,9 +96,9 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m "nhsNumber": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][2][ "NhsNumber" ], - "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE[ - "Items" - ][2]["DocumentSnomedCodeType"], + "documentSnomedCodeType": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][ + 2 + ]["DocumentSnomedCodeType"], "author": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][2]["Author"], "uploadDate": MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"][2][ "UploadDate" @@ -117,12 +113,9 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m ods_code=TEST_CURRENT_GP_ODS, ) - search_document_review_service.decode_start_key.assert_called_with( - TEST_ENCODED_START_KEY - ) search_document_review_service.get_review_document_references.assert_called_with( ods_code=TEST_CURRENT_GP_ODS, - start_key=TEST_LAST_EVALUATED_KEY, + start_key=TEST_ENCODED_START_KEY, limit=int(TEST_QUERY_LIMIT), uploader="Z67890", nhs_number=None, @@ -134,10 +127,6 @@ def test_handle_gateway_api_request_happy_path(search_document_review_service, m def test_process_request_handles_invalid_limit_querystring( search_document_review_service, mocker ): - mocker.patch.object( - search_document_review_service, "decode_start_key" - ).return_value = TEST_LAST_EVALUATED_KEY - with pytest.raises(DocumentReviewLambdaException) as e: search_document_review_service.process_request( params=MOCK_QUERYSTRING_PARAMS_INVALID_LIMIT, ods_code=TEST_CURRENT_GP_ODS @@ -150,10 +139,6 @@ def test_process_request_handles_invalid_limit_querystring( def test_process_request_handles_validation_error( search_document_review_service, mocker ): - mocker.patch.object( - search_document_review_service, "decode_start_key" - ).return_value = TEST_LAST_EVALUATED_KEY - mocker.patch.object( search_document_review_service, "get_review_document_references" ).side_effect = ValidationError("", []) @@ -172,13 +157,13 @@ def test_service_queries_document_review_table_with_correct_args( search_document_review_service, ): search_document_review_service.get_review_document_references( - TEST_CURRENT_GP_ODS, int(TEST_QUERY_LIMIT), TEST_LAST_EVALUATED_KEY, None, None + TEST_CURRENT_GP_ODS, int(TEST_QUERY_LIMIT), TEST_ENCODED_START_KEY, None, None ) - search_document_review_service.document_service.query_docs_pending_review_by_custodian_with_limit.assert_called_with( + search_document_review_service.document_service.query_docs_pending_review_with_paginator.assert_called_with( ods_code=TEST_CURRENT_GP_ODS, limit=int(TEST_QUERY_LIMIT), - start_key=TEST_LAST_EVALUATED_KEY, + start_key=TEST_ENCODED_START_KEY, nhs_number=None, uploader=None, ) @@ -191,9 +176,9 @@ def test_get_review_document_references_returns_document_references( DocumentUploadReviewReference.model_validate(item) for item in MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"] ] - search_document_review_service.document_service.query_docs_pending_review_by_custodian_with_limit.return_value = ( + search_document_review_service.document_service.query_docs_pending_review_with_paginator.return_value = ( expected_references, - TEST_LAST_EVALUATED_KEY, + TEST_ENCODED_START_KEY, ) actual = search_document_review_service.get_review_document_references( @@ -202,7 +187,7 @@ def test_get_review_document_references_returns_document_references( expected = ( expected_references, - TEST_LAST_EVALUATED_KEY, + TEST_ENCODED_START_KEY, ) assert actual == expected @@ -211,7 +196,7 @@ def test_get_review_document_references_returns_document_references( def test_get_review_document_references_handles_empty_result( search_document_review_service, ): - search_document_review_service.document_service.query_docs_pending_review_by_custodian_with_limit.return_value = ( + search_document_review_service.document_service.query_docs_pending_review_with_paginator.return_value = ( [], None, ) @@ -230,7 +215,7 @@ def test_get_review_document_references_handles_no_limit_passed( DocumentUploadReviewReference.model_validate(item) for item in MOCK_DOCUMENT_REVIEW_SEARCH_RESPONSE["Items"] ] - search_document_review_service.document_service.query_docs_pending_review_by_custodian_with_limit.return_value = ( + search_document_review_service.document_service.query_docs_pending_review_with_paginator.return_value = ( expected_references, None, ) @@ -251,7 +236,7 @@ def test_get_review_document_references_throws_exception_client_error( search_document_review_service, ): ( - search_document_review_service.document_service.query_docs_pending_review_by_custodian_with_limit + search_document_review_service.document_service.query_docs_pending_review_with_paginator ).side_effect = DocumentReviewLambdaException(500, LambdaError.DocumentReviewDB) with pytest.raises(DocumentReviewLambdaException) as e: @@ -260,15 +245,3 @@ def test_get_review_document_references_throws_exception_client_error( ) assert e.value.status_code == 500 assert e.value.error == LambdaError.DocumentReviewDB - - -def test_decode_start_key(search_document_review_service): - encoded_start_key = TEST_ENCODED_START_KEY - - actual = search_document_review_service.decode_start_key(encoded_start_key) - assert actual == TEST_LAST_EVALUATED_KEY - - -def test_encode_start_key(search_document_review_service): - actual = search_document_review_service.encode_start_key(TEST_LAST_EVALUATED_KEY) - assert actual == TEST_ENCODED_START_KEY diff --git a/lambdas/tests/unit/utils/test_dynamo_utils.py b/lambdas/tests/unit/utils/test_dynamo_utils.py index 0ebe45ce7..c4bbd9edb 100644 --- a/lambdas/tests/unit/utils/test_dynamo_utils.py +++ b/lambdas/tests/unit/utils/test_dynamo_utils.py @@ -1,4 +1,5 @@ import json +from copy import deepcopy import pytest from enums.lambda_error import LambdaError @@ -26,12 +27,34 @@ create_expression_value_placeholder, create_expressions, create_update_expression, + deserialize_dynamodb_object, parse_dynamo_record, + serialize_dict_to_dynamodb_object, ) from utils.lambda_exceptions import InvalidDocTypeException from lambdas.enums.snomed_codes import SnomedCodes +MOCK_PYTHON_DICT = { + "test_string": "hello", + "test_int": 123, + "test_bool": True, + "test_list": [1, 2, 3], + "test_dict": {"key1": "value1", "key2": 1}, + "test_list_of_dicts": [{"key1": "value1"}, {"key2": 2}], +} + +MOCK_DYNAMO_DB_OBJECT = { + "test_string": {"S": "hello"}, + "test_int": {"N": "123"}, + "test_bool": {"BOOL": True}, + "test_list": {"L": [{"N": "1"}, {"N": "2"}, {"N": "3"}]}, + "test_dict": {"M": {"key1": {"S": "value1"}, "key2": {"N": "1"}}}, + "test_list_of_dicts": { + "L": [{"M": {"key1": {"S": "value1"}}}, {"M": {"key2": {"N": "2"}}}] + }, +} + def test_create_expressions_correctly_creates_an_expression_of_one_field(): expected_projection = "#VirusScannerResult_attr" @@ -528,3 +551,27 @@ def test_build_transaction_item_delete_with_conditions(): delete_item["ExpressionAttributeValues"][":Status_condition_val"] == "archived" ) assert delete_item["ExpressionAttributeValues"][":Expired_condition_val"] is True + + +def test_serialize_dict_to_dynamodb_object(): + input = MOCK_PYTHON_DICT + + expected = MOCK_DYNAMO_DB_OBJECT + actual = serialize_dict_to_dynamodb_object(input) + assert actual == expected + + +def test_deserialize_dynamodb_object(): + input = MOCK_DYNAMO_DB_OBJECT + expected = MOCK_PYTHON_DICT + + actual = deserialize_dynamodb_object(input) + assert actual == expected + + +def test_serialize_dynamodb_object_throws_error_unsupported_data_type(): + unsupported_input = deepcopy(MOCK_PYTHON_DICT) + unsupported_input.update({"float": 1.23}) + + with pytest.raises(TypeError): + serialize_dict_to_dynamodb_object(unsupported_input) diff --git a/lambdas/utils/dynamo_utils.py b/lambdas/utils/dynamo_utils.py index 0f979edb8..336de2910 100644 --- a/lambdas/utils/dynamo_utils.py +++ b/lambdas/utils/dynamo_utils.py @@ -1,7 +1,9 @@ from datetime import datetime +from decimal import Decimal from typing import Any, Dict import inflection +from boto3.dynamodb.types import TypeDeserializer, TypeSerializer from enums.dynamo_filter import AttributeOperator from enums.infrastructure import DynamoTables from enums.lambda_error import LambdaError @@ -87,7 +89,9 @@ def create_expression_attribute_values(attribute_field_values: dict) -> dict: """ expression_attribute_values = {} for field_name, field_value in attribute_field_values.items(): - expression_attribute_values[f"{create_expression_value_placeholder(field_name)}"] = field_value + expression_attribute_values[ + f"{create_expression_value_placeholder(field_name)}" + ] = field_value return expression_attribute_values @@ -131,12 +135,14 @@ def filter_uploaded_docs_and_recently_uploading_docs(): filter_builder.add_condition("Uploaded", AttributeOperator.EQUAL, True) uploaded_filter_expression = filter_builder.build() - filter_builder.add_condition("Uploading", AttributeOperator.EQUAL, True).add_condition( - "LastUpdated", AttributeOperator.GREATER_OR_EQUAL, time_limit - ) + filter_builder.add_condition( + "Uploading", AttributeOperator.EQUAL, True + ).add_condition("LastUpdated", AttributeOperator.GREATER_OR_EQUAL, time_limit) uploading_filter_expression = filter_builder.build() - return delete_filter_expression & (uploaded_filter_expression | uploading_filter_expression) + return delete_filter_expression & ( + uploaded_filter_expression | uploading_filter_expression + ) def parse_dynamo_record(dynamodb_record: Dict[str, Any]) -> Dict[str, Any]: @@ -192,8 +198,12 @@ def build_mixed_condition_expression( if operator in ["attribute_exists", "attribute_not_exists"]: condition_expressions.append(f"{operator}({condition_placeholder})") else: - condition_value_placeholder = create_expression_value_placeholder(field_name, suffix) - condition_expressions.append(f"{condition_placeholder} {operator} {condition_value_placeholder}") + condition_value_placeholder = create_expression_value_placeholder( + field_name, suffix + ) + condition_expressions.append( + f"{condition_placeholder} {operator} {condition_value_placeholder}" + ) condition_attribute_values[condition_value_placeholder] = field_value condition_expression = f" {join_operator} ".join(condition_expressions) @@ -234,7 +244,9 @@ def build_general_transaction_item( action = action.capitalize() if action not in ["Put", "Update", "Delete", "Conditioncheck"]: - raise ValueError(f"Invalid action: {action}. Must be one of: Put, Update, Delete, ConditionCheck") + raise ValueError( + f"Invalid action: {action}. Must be one of: Put, Update, Delete, ConditionCheck" + ) transaction_item: dict[str, dict[str, Any]] = {action: {"TableName": table_name}} @@ -259,10 +271,14 @@ def build_general_transaction_item( transaction_item[action]["ConditionExpression"] = condition_expression if expression_attribute_names: - transaction_item[action]["ExpressionAttributeNames"] = expression_attribute_names + transaction_item[action][ + "ExpressionAttributeNames" + ] = expression_attribute_names if expression_attribute_values: - transaction_item[action]["ExpressionAttributeValues"] = expression_attribute_values + transaction_item[action][ + "ExpressionAttributeValues" + ] = expression_attribute_values return transaction_item @@ -291,8 +307,8 @@ def build_transaction_item( expression_attribute_values.update(update_attr_values) if conditions: - condition_expr, condition_attr_names, condition_attr_values = build_mixed_condition_expression( - conditions, condition_join_operator + condition_expr, condition_attr_names, condition_attr_values = ( + build_mixed_condition_expression(conditions, condition_join_operator) ) condition_expression = condition_expr expression_attribute_names.update(condition_attr_names) @@ -310,6 +326,21 @@ def build_transaction_item( ) +def serialize_dict_to_dynamodb_object(object: dict) -> dict: + serializer = TypeSerializer() + return {key: serializer.serialize(value) for key, value in object.items()} + + +def deserialize_dynamodb_object(object: dict) -> dict: + deserialize = TypeDeserializer().deserialize + parsed_dynamodb_items = {} + for key, value in object.items(): + parsed_dynamodb_items[key] = deserialize(value) + if type(deserialize(value)) is Decimal: + parsed_dynamodb_items[key] = int(deserialize(value)) + return parsed_dynamodb_items + + class DocTypeTableRouter: def __init__(self): self._define_tables() @@ -329,5 +360,7 @@ def resolve(self, doc_type: SnomedCode) -> str: table = self.mapping[doc_type.code] return str(table) except KeyError: - logger.error(f"SNOMED code {doc_type.code} - {doc_type.display_name} is not supported") + logger.error( + f"SNOMED code {doc_type.code} - {doc_type.display_name} is not supported" + ) raise InvalidDocTypeException(400, LambdaError.DocTypeDB)