From 0e0124781cee103a9e2badacdd99beb3ef8340b2 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Wed, 21 Jul 2021 21:12:26 -0700 Subject: [PATCH 1/5] fix(parser): apigw check_message_id and tests Changes: - Add some more unit tests - Fix bug with `check_message_id` not looking in the RequestContext - Add more mypy coverage - Add pragma: no cover --- aws_lambda_powertools/tracing/tracer.py | 4 ++-- .../feature_toggles/appconfig_fetcher.py | 16 +++++++++------ .../idempotency/persistence/dynamodb.py | 2 +- .../utilities/parameters/dynamodb.py | 11 +++------- .../utilities/parser/models/apigw.py | 14 ++++++------- tests/functional/parser/test_apigw.py | 20 +++++++++++++++++++ 6 files changed, 43 insertions(+), 24 deletions(-) diff --git a/aws_lambda_powertools/tracing/tracer.py b/aws_lambda_powertools/tracing/tracer.py index 5709b1956c2..bd4244b3171 100644 --- a/aws_lambda_powertools/tracing/tracer.py +++ b/aws_lambda_powertools/tracing/tracer.py @@ -335,7 +335,7 @@ def decorate(event, context, **kwargs): # see #465 @overload def capture_method(self, method: "AnyCallableT") -> "AnyCallableT": - ... + ... # pragma: no cover @overload def capture_method( @@ -344,7 +344,7 @@ def capture_method( capture_response: Optional[bool] = None, capture_error: Optional[bool] = None, ) -> Callable[["AnyCallableT"], "AnyCallableT"]: - ... + ... # pragma: no cover def capture_method( self, diff --git a/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py b/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py index ae7c6c90e51..3501edfd0d3 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py +++ b/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py @@ -1,5 +1,5 @@ import logging -from typing import Any, Dict, Optional +from typing import Any, Dict, Optional, cast from botocore.config import Config @@ -56,11 +56,15 @@ def get_json_configuration(self) -> Dict[str, Any]: parsed JSON dictionary """ try: - return self._conf_store.get( - name=self.configuration_name, - transform=TRANSFORM_TYPE, - max_age=self._cache_seconds, - ) # parse result conf as JSON, keep in cache for self.max_age seconds + # parse result conf as JSON, keep in cache for self.max_age seconds + return cast( + dict, + self._conf_store.get( + name=self.configuration_name, + transform=TRANSFORM_TYPE, + max_age=self._cache_seconds, + ), + ) except (GetParameterError, TransformParameterError) as exc: error_str = f"unable to get AWS AppConfig configuration file, exception={str(exc)}" self._logger.error(error_str) diff --git a/aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py b/aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py index dc00334277e..ae3a1be490f 100644 --- a/aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py +++ b/aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py @@ -154,7 +154,7 @@ def _update_record(self, data_record: DataRecord): "ExpressionAttributeNames": expression_attr_names, } - self.table.update_item(**kwargs) + self.table.update_item(**kwargs) # type: ignore def _delete_record(self, data_record: DataRecord) -> None: logger.debug(f"Deleting record for idempotency key: {data_record.idempotency_key}") diff --git a/aws_lambda_powertools/utilities/parameters/dynamodb.py b/aws_lambda_powertools/utilities/parameters/dynamodb.py index 5edae643ec0..e7e6c75e213 100644 --- a/aws_lambda_powertools/utilities/parameters/dynamodb.py +++ b/aws_lambda_powertools/utilities/parameters/dynamodb.py @@ -3,7 +3,7 @@ """ -from typing import Any, Dict, Optional +from typing import Dict, Optional import boto3 from boto3.dynamodb.conditions import Key @@ -141,11 +141,6 @@ class DynamoDBProvider(BaseProvider): c Parameter value c """ - table: Any = None - key_attr = None - sort_attr = None - value_attr = None - def __init__( self, table_name: str, @@ -183,7 +178,7 @@ def _get(self, name: str, **sdk_options) -> str: # Explicit arguments will take precedence over keyword arguments sdk_options["Key"] = {self.key_attr: name} - return self.table.get_item(**sdk_options)["Item"][self.value_attr] + return str(self.table.get_item(**sdk_options)["Item"][self.value_attr]) def _get_multiple(self, path: str, **sdk_options) -> Dict[str, str]: """ @@ -209,4 +204,4 @@ def _get_multiple(self, path: str, **sdk_options) -> Dict[str, str]: response = self.table.query(**sdk_options) items.extend(response.get("Items", [])) - return {item[self.sort_attr]: item[self.value_attr] for item in items} + return {str(item[self.sort_attr]): str(item[self.value_attr]) for item in items} diff --git a/aws_lambda_powertools/utilities/parser/models/apigw.py b/aws_lambda_powertools/utilities/parser/models/apigw.py index 4de8ee96cc5..2da93e97b81 100644 --- a/aws_lambda_powertools/utilities/parser/models/apigw.py +++ b/aws_lambda_powertools/utilities/parser/models/apigw.py @@ -68,6 +68,13 @@ class APIGatewayEventRequestContext(BaseModel): routeKey: Optional[str] operationName: Optional[str] + @root_validator() + def check_message_id(cls, values): + message_id, event_type = values.get("messageId"), values.get("eventType") + if message_id is not None and event_type != "MESSAGE": + raise TypeError("messageId is available only when the `eventType` is `MESSAGE`") + return values + class APIGatewayProxyEventModel(BaseModel): version: Optional[str] @@ -83,10 +90,3 @@ class APIGatewayProxyEventModel(BaseModel): stageVariables: Optional[Dict[str, str]] isBase64Encoded: bool body: str - - @root_validator() - def check_message_id(cls, values): - message_id, event_type = values.get("messageId"), values.get("eventType") - if message_id is not None and event_type != "MESSAGE": - raise TypeError("messageId is available only when the `eventType` is `MESSAGE`") - return values diff --git a/tests/functional/parser/test_apigw.py b/tests/functional/parser/test_apigw.py index fc679d5dc37..5c92fe68af9 100644 --- a/tests/functional/parser/test_apigw.py +++ b/tests/functional/parser/test_apigw.py @@ -1,3 +1,6 @@ +import pytest +from pydantic import ValidationError + from aws_lambda_powertools.utilities.parser import envelopes, event_parser from aws_lambda_powertools.utilities.parser.models import APIGatewayProxyEventModel from aws_lambda_powertools.utilities.typing import LambdaContext @@ -100,3 +103,20 @@ def test_apigw_event(): assert request_context.operationName is None assert identity.apiKey is None assert identity.apiKeyId is None + + +def test_apigw_event_with_invalid_websocket_request(): + # GIVEN an event with an eventType != MESSAGE and has a messageId + event = { + "requestContext": { + "eventType": "DISCONNECT", + "messageId": "messageId", + }, + } + + # WHEN calling event_parser with APIGatewayProxyEventModel + with pytest.raises(ValidationError) as err: + handle_apigw_event(event, LambdaContext()) + + # THEN raise TypeError for invalid event + assert "messageId is available only when the `eventType` is `MESSAGE`" in str(err.value) From 7b896bee3832b75031dcd5268d4ff11efda1e26a Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Wed, 21 Jul 2021 23:20:50 -0700 Subject: [PATCH 2/5] chore: recommended updates --- .../utilities/parser/models/apigw.py | 2 +- tests/functional/parser/test_apigw.py | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/aws_lambda_powertools/utilities/parser/models/apigw.py b/aws_lambda_powertools/utilities/parser/models/apigw.py index 2da93e97b81..ed975e88e81 100644 --- a/aws_lambda_powertools/utilities/parser/models/apigw.py +++ b/aws_lambda_powertools/utilities/parser/models/apigw.py @@ -68,7 +68,7 @@ class APIGatewayEventRequestContext(BaseModel): routeKey: Optional[str] operationName: Optional[str] - @root_validator() + @root_validator def check_message_id(cls, values): message_id, event_type = values.get("messageId"), values.get("eventType") if message_id is not None and event_type != "MESSAGE": diff --git a/tests/functional/parser/test_apigw.py b/tests/functional/parser/test_apigw.py index 5c92fe68af9..016ec4422bc 100644 --- a/tests/functional/parser/test_apigw.py +++ b/tests/functional/parser/test_apigw.py @@ -108,7 +108,27 @@ def test_apigw_event(): def test_apigw_event_with_invalid_websocket_request(): # GIVEN an event with an eventType != MESSAGE and has a messageId event = { + "resource": "/", + "path": "/", + "httpMethod": "GET", + "headers": {}, + "multiValueHeaders": {}, + "isBase64Encoded": False, + "body": "Foo!", "requestContext": { + "accountId": "1234", + "apiId": "myApi", + "httpMethod": "GET", + "identity": { + "sourceIp": "127.0.0.1", + }, + "path": "/", + "protocol": "Https", + "requestId": "1234", + "requestTime": "2018-09-07T16:20:46Z", + "requestTimeEpoch": 1536992496000, + "resourcePath": "/", + "stage": "test", "eventType": "DISCONNECT", "messageId": "messageId", }, @@ -119,4 +139,5 @@ def test_apigw_event_with_invalid_websocket_request(): handle_apigw_event(event, LambdaContext()) # THEN raise TypeError for invalid event + assert len(err.value.errors()) == 1 assert "messageId is available only when the `eventType` is `MESSAGE`" in str(err.value) From 02e70e4164a3541beea4b97535c17b0c87e421d9 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Thu, 22 Jul 2021 07:57:39 -0700 Subject: [PATCH 3/5] chore: more detailed comparison --- tests/functional/parser/test_apigw.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/functional/parser/test_apigw.py b/tests/functional/parser/test_apigw.py index 016ec4422bc..d657a0dbe4d 100644 --- a/tests/functional/parser/test_apigw.py +++ b/tests/functional/parser/test_apigw.py @@ -139,5 +139,8 @@ def test_apigw_event_with_invalid_websocket_request(): handle_apigw_event(event, LambdaContext()) # THEN raise TypeError for invalid event - assert len(err.value.errors()) == 1 - assert "messageId is available only when the `eventType` is `MESSAGE`" in str(err.value) + errors = err.value.errors() + assert len(errors) == 1 + expected_msg = "messageId is available only when the `eventType` is `MESSAGE`" + assert errors[0]["msg"] == expected_msg + assert expected_msg in str(err.value) From 4b44bd49d53e462b34cd5969409f8844a7666588 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Fri, 23 Jul 2021 14:59:58 -0700 Subject: [PATCH 4/5] chore(idempotency): simplify max_handler_retries --- aws_lambda_powertools/utilities/idempotency/idempotency.py | 5 +---- tests/functional/idempotency/test_idempotency.py | 1 - 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/aws_lambda_powertools/utilities/idempotency/idempotency.py b/aws_lambda_powertools/utilities/idempotency/idempotency.py index c2bcc62fd69..fc1d4d47d55 100644 --- a/aws_lambda_powertools/utilities/idempotency/idempotency.py +++ b/aws_lambda_powertools/utilities/idempotency/idempotency.py @@ -78,9 +78,7 @@ def idempotent( try: return idempotency_handler.handle() except IdempotencyInconsistentStateError: - if i < max_handler_retries: - continue - else: + if i == max_handler_retries: # Allow the exception to bubble up after max retries exceeded raise @@ -117,7 +115,6 @@ def __init__( self.context = context self.event = event self.lambda_handler = lambda_handler - self.max_handler_retries = 2 def handle(self) -> Any: """ diff --git a/tests/functional/idempotency/test_idempotency.py b/tests/functional/idempotency/test_idempotency.py index 0cf19ab9de0..0ecc84b7f9c 100644 --- a/tests/functional/idempotency/test_idempotency.py +++ b/tests/functional/idempotency/test_idempotency.py @@ -395,7 +395,6 @@ def test_idempotent_lambda_expired_during_request( lambda_apigw_event, timestamp_expired, lambda_response, - expected_params_update_item, hashed_idempotency_key, lambda_context, ): From 3e8224ef92be07f72e5d790519ddcf93ece11116 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Mon, 26 Jul 2021 00:22:07 -0700 Subject: [PATCH 5/5] chore: revert str cast --- aws_lambda_powertools/utilities/parameters/dynamodb.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aws_lambda_powertools/utilities/parameters/dynamodb.py b/aws_lambda_powertools/utilities/parameters/dynamodb.py index e7e6c75e213..39bd1a8d6b7 100644 --- a/aws_lambda_powertools/utilities/parameters/dynamodb.py +++ b/aws_lambda_powertools/utilities/parameters/dynamodb.py @@ -178,7 +178,7 @@ def _get(self, name: str, **sdk_options) -> str: # Explicit arguments will take precedence over keyword arguments sdk_options["Key"] = {self.key_attr: name} - return str(self.table.get_item(**sdk_options)["Item"][self.value_attr]) + return self.table.get_item(**sdk_options)["Item"][self.value_attr] def _get_multiple(self, path: str, **sdk_options) -> Dict[str, str]: """ @@ -204,4 +204,4 @@ def _get_multiple(self, path: str, **sdk_options) -> Dict[str, str]: response = self.table.query(**sdk_options) items.extend(response.get("Items", [])) - return {str(item[self.sort_attr]): str(item[self.value_attr]) for item in items} + return {item[self.sort_attr]: item[self.value_attr] for item in items}