From 8308fe15a62d333f626cd9acb69af10d67dcb001 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Sun, 21 Feb 2021 19:04:04 -0800 Subject: [PATCH 1/6] feat(idempotencty): Raise error when event data is None GIVEN a persistence_store with event_key_jmespath = `body` WHEN getting the hashed idempotency key with an event without a `body` key THEN raise IdempotencyValidationError --- .../utilities/idempotency/persistence/base.py | 4 ++++ tests/functional/idempotency/conftest.py | 2 +- tests/functional/idempotency/test_idempotency.py | 14 ++++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/aws_lambda_powertools/utilities/idempotency/persistence/base.py b/aws_lambda_powertools/utilities/idempotency/persistence/base.py index c3183e0df84..1e8423192da 100644 --- a/aws_lambda_powertools/utilities/idempotency/persistence/base.py +++ b/aws_lambda_powertools/utilities/idempotency/persistence/base.py @@ -162,6 +162,10 @@ def _get_hashed_idempotency_key(self, lambda_event: Dict[str, Any]) -> str: data = lambda_event if self.event_key_jmespath: data = self.event_key_compiled_jmespath.search(lambda_event) + + if data is None: + raise IdempotencyValidationError("No data found to create a hashed idempotency_key") + return self._generate_hash(data) def _get_hashed_payload(self, lambda_event: Dict[str, Any]) -> str: diff --git a/tests/functional/idempotency/conftest.py b/tests/functional/idempotency/conftest.py index 918eac9a507..68492648337 100644 --- a/tests/functional/idempotency/conftest.py +++ b/tests/functional/idempotency/conftest.py @@ -152,7 +152,7 @@ def hashed_validation_key(lambda_apigw_event): @pytest.fixture def persistence_store(config, request, default_jmespath): persistence_store = DynamoDBPersistenceLayer( - event_key_jmespath=default_jmespath, + event_key_jmespath=request.param.get("event_key_jmespath") or default_jmespath, table_name=TABLE_NAME, boto_config=config, use_local_cache=request.param["use_local_cache"], diff --git a/tests/functional/idempotency/test_idempotency.py b/tests/functional/idempotency/test_idempotency.py index 269ab6f9b33..10502a08d7c 100644 --- a/tests/functional/idempotency/test_idempotency.py +++ b/tests/functional/idempotency/test_idempotency.py @@ -627,3 +627,17 @@ def test_user_local_disabled(persistence_store): # THEN raise AttributeError # AND don't have a _cache attribute assert not hasattr("persistence_store", "_cache") + + +@pytest.mark.parametrize("persistence_store", [{"use_local_cache": False, "event_key_jmespath": "body"}], indirect=True) +def test_no_data_for_generate_hash(persistence_store): + # GIVEN a persistence_store with use_local_cache = False and event_key_jmespath = "body" + assert persistence_store.use_local_cache is False + assert "body" in persistence_store.event_key_jmespath + + # WHEN getting the hashed idempotency key for an event with no `body` key + with pytest.raises(IdempotencyValidationError) as excinfo: + persistence_store._get_hashed_idempotency_key({}) + + # THEN raise IdempotencyValidationError error + assert "No data found to create a hashed idempotency_key" in str(excinfo.value) From fc8b053e7c2fd855197d342f379f59a3bc974d5b Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Thu, 25 Feb 2021 03:30:51 -0800 Subject: [PATCH 2/6] feat(idempotency): Add raise_on_no_idempotency_key Add `raise_on_no_idempotency_key` to enforce a idempotency key value is passed into the lambda. --- .../utilities/idempotency/exceptions.py | 6 +++ .../utilities/idempotency/persistence/base.py | 16 +++++++- docs/utilities/idempotency.md | 16 +++++++- .../idempotency/test_idempotency.py | 38 +++++++++++++++++-- 4 files changed, 69 insertions(+), 7 deletions(-) diff --git a/aws_lambda_powertools/utilities/idempotency/exceptions.py b/aws_lambda_powertools/utilities/idempotency/exceptions.py index 1d7a8acab1f..6c7318ebca0 100644 --- a/aws_lambda_powertools/utilities/idempotency/exceptions.py +++ b/aws_lambda_powertools/utilities/idempotency/exceptions.py @@ -43,3 +43,9 @@ class IdempotencyPersistenceLayerError(Exception): """ Unrecoverable error from the data store """ + + +class IdempotencyKeyError(Exception): + """ + Payload does not contain a idempotent key + """ diff --git a/aws_lambda_powertools/utilities/idempotency/persistence/base.py b/aws_lambda_powertools/utilities/idempotency/persistence/base.py index 1e8423192da..4f51d59c6bf 100644 --- a/aws_lambda_powertools/utilities/idempotency/persistence/base.py +++ b/aws_lambda_powertools/utilities/idempotency/persistence/base.py @@ -6,6 +6,7 @@ import hashlib import json import logging +import warnings from abc import ABC, abstractmethod from types import MappingProxyType from typing import Any, Dict @@ -17,6 +18,7 @@ from aws_lambda_powertools.utilities.idempotency.exceptions import ( IdempotencyInvalidStatusError, IdempotencyItemAlreadyExistsError, + IdempotencyKeyError, IdempotencyValidationError, ) @@ -112,6 +114,7 @@ def __init__( use_local_cache: bool = False, local_cache_max_items: int = 256, hash_function: str = "md5", + raise_on_no_idempotency_key: bool = False, ) -> None: """ Initialize the base persistence layer @@ -130,6 +133,8 @@ def __init__( Max number of items to store in local cache, by default 1024 hash_function: str, optional Function to use for calculating hashes, by default md5. + raise_on_no_idempotency_key: bool, optional + Raise exception if no idempotency key was found in the request, by default False """ self.event_key_jmespath = event_key_jmespath if self.event_key_jmespath: @@ -143,6 +148,7 @@ def __init__( self.validation_key_jmespath = jmespath.compile(payload_validation_jmespath) self.payload_validation_enabled = True self.hash_function = getattr(hashlib, hash_function) + self.raise_on_no_idempotency_key = raise_on_no_idempotency_key def _get_hashed_idempotency_key(self, lambda_event: Dict[str, Any]) -> str: """ @@ -163,11 +169,17 @@ def _get_hashed_idempotency_key(self, lambda_event: Dict[str, Any]) -> str: if self.event_key_jmespath: data = self.event_key_compiled_jmespath.search(lambda_event) - if data is None: - raise IdempotencyValidationError("No data found to create a hashed idempotency_key") + if self.is_missing_idempotency_key(data): + warnings.warn(f"No value found for idempotency_key. jmespath: {self.event_key_jmespath}") + if self.raise_on_no_idempotency_key: + raise IdempotencyKeyError("No data found to create a hashed idempotency_key") return self._generate_hash(data) + @staticmethod + def is_missing_idempotency_key(data) -> bool: + return data is None or (type(data) is list and all(x is None for x in data)) + def _get_hashed_payload(self, lambda_event: Dict[str, Any]) -> str: """ Extract data from lambda event using validation key jmespath, and return a hashed representation diff --git a/docs/utilities/idempotency.md b/docs/utilities/idempotency.md index 6bc7457d603..c1c1c3f6235 100644 --- a/docs/utilities/idempotency.md +++ b/docs/utilities/idempotency.md @@ -259,6 +259,20 @@ payload validation, we would have returned the same result as we did for the ini returning an amount in the response, this could be quite confusing for the client. By using payload validation on the amount field, we prevent this potentially confusing behaviour and instead raise an Exception. +### Making idempotency key required + +By default, events without any idempotency key don't raise any exception and just trigger a warning. +If you want to ensure that at an idempotency is found, you can pass in `raise_on_no_idempotency_key` as True and an +`IdempotencyKeyError` will be raised. + +```python hl_lines="4" +DynamoDBPersistenceLayer( + event_key_jmespath="body", + table_name="IdempotencyTable", + raise_on_no_idempotency_key=True + ) +``` + ### Changing dynamoDB attribute names If you want to use an existing DynamoDB table, or wish to change the name of the attributes used to store items in the table, you can do so when you construct the `DynamoDBPersistenceLayer` instance. @@ -278,7 +292,7 @@ This example demonstrates changing the attribute names to custom values: ```python hl_lines="5-10" persistence_layer = DynamoDBPersistenceLayer( event_key_jmespath="[userDetail, productId]", - table_name="IdempotencyTable",) + table_name="IdempotencyTable", key_attr="idempotency_key", expiry_attr="expires_at", status_attr="current_status", diff --git a/tests/functional/idempotency/test_idempotency.py b/tests/functional/idempotency/test_idempotency.py index 10502a08d7c..34bbba67c24 100644 --- a/tests/functional/idempotency/test_idempotency.py +++ b/tests/functional/idempotency/test_idempotency.py @@ -1,5 +1,7 @@ import copy +import json import sys +from hashlib import md5 import pytest from botocore import stub @@ -8,11 +10,12 @@ IdempotencyAlreadyInProgressError, IdempotencyInconsistentStateError, IdempotencyInvalidStatusError, + IdempotencyKeyError, IdempotencyPersistenceLayerError, IdempotencyValidationError, ) from aws_lambda_powertools.utilities.idempotency.idempotency import idempotent -from aws_lambda_powertools.utilities.idempotency.persistence.base import DataRecord +from aws_lambda_powertools.utilities.idempotency.persistence.base import BasePersistenceLayer, DataRecord from aws_lambda_powertools.utilities.validation import envelopes, validator TABLE_NAME = "TEST_TABLE" @@ -629,15 +632,42 @@ def test_user_local_disabled(persistence_store): assert not hasattr("persistence_store", "_cache") +def test_is_missing_idempotency_key(): + # GIVEN None THEN is_missing_idempotency_key is True + assert BasePersistenceLayer.is_missing_idempotency_key(None) + # GIVEN a list of Nones THEN is_missing_idempotency_key is True + assert BasePersistenceLayer.is_missing_idempotency_key([None, None]) + # GIVEN a list of all not None THEN is_missing_idempotency_key is false + assert BasePersistenceLayer.is_missing_idempotency_key([None, "Value"]) is False + # GIVEN a str THEN is_missing_idempotency_key is false + assert BasePersistenceLayer.is_missing_idempotency_key("Value") is False + + @pytest.mark.parametrize("persistence_store", [{"use_local_cache": False, "event_key_jmespath": "body"}], indirect=True) -def test_no_data_for_generate_hash(persistence_store): +def test_default_no_raise_on_missing_idempotency_key(persistence_store): # GIVEN a persistence_store with use_local_cache = False and event_key_jmespath = "body" assert persistence_store.use_local_cache is False assert "body" in persistence_store.event_key_jmespath # WHEN getting the hashed idempotency key for an event with no `body` key - with pytest.raises(IdempotencyValidationError) as excinfo: + hashed_key = persistence_store._get_hashed_idempotency_key({}) + + # THEN return the hash of None + assert md5(json.dumps(None).encode()).hexdigest() == hashed_key + + +@pytest.mark.parametrize( + "persistence_store", [{"use_local_cache": False, "event_key_jmespath": "[body, x]"}], indirect=True +) +def test_raise_on_no_idempotency_key(persistence_store): + # GIVEN a persistence_store with raise_on_no_idempotency_key and no idempotency key in the request + persistence_store.raise_on_no_idempotency_key = True + assert persistence_store.use_local_cache is False + assert "body" in persistence_store.event_key_jmespath + + # WHEN getting the hashed idempotency key for an event with no `body` key + with pytest.raises(IdempotencyKeyError) as excinfo: persistence_store._get_hashed_idempotency_key({}) - # THEN raise IdempotencyValidationError error + # THEN raise IdempotencyKeyError error assert "No data found to create a hashed idempotency_key" in str(excinfo.value) From 6fa4824050032133e3209d1366e36f5e48c3524b Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Wed, 3 Mar 2021 05:14:47 -0800 Subject: [PATCH 3/6] fix: Include empty data structures Take in account for empty data structures, including non-lists iterables like tuples, dictionaries Co-authored-by: Heitor Lessa --- aws_lambda_powertools/utilities/idempotency/persistence/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws_lambda_powertools/utilities/idempotency/persistence/base.py b/aws_lambda_powertools/utilities/idempotency/persistence/base.py index da55a9e8b3a..c866d75d98d 100644 --- a/aws_lambda_powertools/utilities/idempotency/persistence/base.py +++ b/aws_lambda_powertools/utilities/idempotency/persistence/base.py @@ -178,7 +178,7 @@ def _get_hashed_idempotency_key(self, lambda_event: Dict[str, Any]) -> str: @staticmethod def is_missing_idempotency_key(data) -> bool: - return data is None or (type(data) is list and all(x is None for x in data)) + return data is None or not data or all(x is None for x in data) def _get_hashed_payload(self, lambda_event: Dict[str, Any]) -> str: """ From fd041934c55521229a39e08d4e9c1691f749b7e2 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Wed, 3 Mar 2021 05:15:15 -0800 Subject: [PATCH 4/6] tests: Include tests for empty data structures Co-authored-by: Heitor Lessa --- tests/functional/idempotency/test_idempotency.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/functional/idempotency/test_idempotency.py b/tests/functional/idempotency/test_idempotency.py index fda5d64c239..29c9feb0f3d 100644 --- a/tests/functional/idempotency/test_idempotency.py +++ b/tests/functional/idempotency/test_idempotency.py @@ -653,6 +653,9 @@ def test_is_missing_idempotency_key(): # GIVEN a str THEN is_missing_idempotency_key is false assert BasePersistenceLayer.is_missing_idempotency_key("Value") is False + assert BasePersistenceLayer.is_missing_idempotency_key(()) is False + # GIVEN an empty dictionary THEN is_missing_idempotency_key is false + assert BasePersistenceLayer.is_missing_idempotency_key({}) is False @pytest.mark.parametrize("persistence_store", [{"use_local_cache": False, "event_key_jmespath": "body"}], indirect=True) def test_default_no_raise_on_missing_idempotency_key(persistence_store): From 03176b1c16bb99fe979cb8af81875319dedd6bf0 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Wed, 3 Mar 2021 05:21:40 -0800 Subject: [PATCH 5/6] fix: tests --- tests/functional/idempotency/test_idempotency.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/functional/idempotency/test_idempotency.py b/tests/functional/idempotency/test_idempotency.py index 29c9feb0f3d..675d8d1c5c8 100644 --- a/tests/functional/idempotency/test_idempotency.py +++ b/tests/functional/idempotency/test_idempotency.py @@ -652,10 +652,15 @@ def test_is_missing_idempotency_key(): assert BasePersistenceLayer.is_missing_idempotency_key([None, "Value"]) is False # GIVEN a str THEN is_missing_idempotency_key is false assert BasePersistenceLayer.is_missing_idempotency_key("Value") is False - - assert BasePersistenceLayer.is_missing_idempotency_key(()) is False + # GIVEN an empty tuple THEN is_missing_idempotency_key is false + assert BasePersistenceLayer.is_missing_idempotency_key(()) + # GIVEN an empty list THEN is_missing_idempotency_key is false + assert BasePersistenceLayer.is_missing_idempotency_key([]) # GIVEN an empty dictionary THEN is_missing_idempotency_key is false - assert BasePersistenceLayer.is_missing_idempotency_key({}) is False + assert BasePersistenceLayer.is_missing_idempotency_key({}) + # GIVEN an empty str THEN is_missing_idempotency_key is false + assert BasePersistenceLayer.is_missing_idempotency_key("") + @pytest.mark.parametrize("persistence_store", [{"use_local_cache": False, "event_key_jmespath": "body"}], indirect=True) def test_default_no_raise_on_missing_idempotency_key(persistence_store): From 351846c00c0ff262f1ee5da9ba4d35c3a1ad1043 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Wed, 3 Mar 2021 05:33:55 -0800 Subject: [PATCH 6/6] docs: Make some doc changes --- docs/utilities/idempotency.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/utilities/idempotency.md b/docs/utilities/idempotency.md index c1c1c3f6235..1193663b2e8 100644 --- a/docs/utilities/idempotency.md +++ b/docs/utilities/idempotency.md @@ -254,16 +254,15 @@ idempotent invocations. ``` In this example, the "userDetail" and "productId" keys are used as the payload to generate the idempotency key. If -we try to send the same request but with a different amount, Lambda will raise `IdempotencyValidationError`. Without +we try to send the same request but with a different amount, we will raise `IdempotencyValidationError`. Without payload validation, we would have returned the same result as we did for the initial request. Since we're also returning an amount in the response, this could be quite confusing for the client. By using payload validation on the amount field, we prevent this potentially confusing behaviour and instead raise an Exception. ### Making idempotency key required -By default, events without any idempotency key don't raise any exception and just trigger a warning. -If you want to ensure that at an idempotency is found, you can pass in `raise_on_no_idempotency_key` as True and an -`IdempotencyKeyError` will be raised. +If you want to enforce that an idempotency key is required, you can set `raise_on_no_idempotency_key` to `True`, +and we will raise `IdempotencyKeyError` if none was found. ```python hl_lines="4" DynamoDBPersistenceLayer(