Skip to content

Commit 08c9921

Browse files
fix(idempotency): resolve tech debt issues with falsy responses and Redis persistency (#8176)
* fix(idempotency): fix str(None) producing "None" string in Redis persistence Replace str(item.get(...)) with item.get(...) to avoid storing the string "None" when a value is missing from Redis hash map. When data_attr or validation_key_attr is missing from Redis, item.get() returns None. Wrapping with str() converts it to the string "None" which is incorrect. Now correctly returns None. Part of #8090 Signed-off-by: hirenkumar-n-dholariya <hirenkumarnd@gmail.com> * refactor(idempotency): extract duplicated idempotency key null-check into helper method Replace 4 identical null-check blocks across save_success, save_inprogress, delete_record, and get_record with a single helper method _get_idempotency_key_or_return_none() to reduce code duplication. The helper encapsulates the pattern of calling _get_hashed_idempotency_key() and returning None early if the key is None, keeping each method cleaner and easier to read. Part of #8090 Signed-off-by: hirenkumar-n-dholariya <hirenkumarnd@gmail.com> * test(idempotency): add unit tests for tech debt fixes in issue #8090 Fix 1 - str(None) in Redis _item_to_data_record: - Missing data_attr returns None not string "None" - Existing data_attr returns value correctly - Demonstrates old bug vs new correct behavior Fix 2 - _get_idempotency_key_or_return_none helper: - Returns None when key is None - Returns key string when key exists - Correctly used in save_success, save_inprogress, delete_record, and get_record (all return None early) Part of #8090 Signed-off-by: hirenkumar-n-dholariya <hirenkumarnd@gmail.com> * fix(idempotency): fix falsy response handling and inconsistent status constant Fix 1 - Falsy response handling in _get_function_response(): Replace `if response else None` with `if response is not None else None`. So valid falsy return values (0, False, {}, [], "") are correctly serialized and stored instead of being silently discarded. Fix 2 - Inconsistent status constant in _process_idempotency(): Replace string literal "INPROGRESS" with STATUS_CONSTANTS["INPROGRESS"] for consistency with the rest of the codebase. Part of #8090 Signed-off-by: hirenkumar-n-dholariya <hirenkumarnd@gmail.com> * refactor(idempotency): revert helper method - restore original inline null-check pattern Per Leandro Damascena's feedback: _get_idempotency_key_or_return_none() helper added indirection without reducing duplication since the if None: return None check remained in all 4 callers anyway. Restored original inline 3-line pattern in save_success, save_inprogress, delete_record, and get_record which is clearer and instantly readable. Part of #8090 Signed-off-by: hirenkumar-n-dholariya <hirenkumarnd@gmail.com> * test(idempotency): remove standalone test file per maintainer feedback Tests should be added to existing test files following established patterns, not in new standalone files. The Redis fix will be tested in _redis/test_redis_layer.py next to the existing test_item_to_datarecord_conversion. Part of #8090 Signed-off-by: hirenkumar-n-dholariya <hirenkumarnd@gmail.com> * test(idempotency): add regression test for str(None) fix in _item_to_data_record Added single test next to existing test_item_to_datarecord_conversion to verify missing Redis attributes return None instead of string "None". Follows existing test patterns using fixtures instead of MagicMock. Part of #8090 Signed-off-by: hirenkumar-n-dholariya <hirenkumarnd@gmail.com> * fix(idempotency): fix ruff formatting - remove trailing whitespace in base.py Remove trailing whitespace on blank line between is_missing_idempotency_key and _get_hashed_payload methods to pass ruff format check. Part of #8090 Signed-off-by: hirenkumar-n-dholariya <hirenkumarnd@gmail.com> * fix(idempotency): fix test fixture name and trailing whitespace in test_redis_layer.py - Fix fixture name from persistence_store_redis to persistence_store_standalone_redis to match existing fixture defined in the test file - Remove trailing whitespace on blank line after test function to pass ruff format check Part of #8090 Signed-off-by: hirenkumar-n-dholariya <hirenkumarnd@gmail.com> * fix: small changes --------- Signed-off-by: hirenkumar-n-dholariya <hirenkumarnd@gmail.com> Co-authored-by: Leandro Damascena <lcdama@amazon.pt>
1 parent 0834363 commit 08c9921

3 files changed

Lines changed: 23 additions & 4 deletions

File tree

aws_lambda_powertools/utilities/idempotency/base.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ def _process_idempotency(self, is_replay: bool):
167167
# We give preference to ReturnValuesOnConditionCheckFailure because it is a faster and more cost-effective
168168
# way of retrieving the existing record after a failed conditional write operation.
169169
record = exc.old_data_record or self._get_idempotency_record()
170-
if is_replay and record is not None and record.status == "INPROGRESS":
170+
if is_replay and record is not None and record.status == STATUS_CONSTANTS["INPROGRESS"]:
171171
return self._get_function_response()
172172
# If a record is found, handle it for status
173173
if record:
@@ -296,7 +296,7 @@ def _get_function_response(self):
296296

297297
else:
298298
try:
299-
serialized_response: dict = self.output_serializer.to_dict(response) if response else None
299+
serialized_response: dict = self.output_serializer.to_dict(response) if response is not None else None
300300
self.persistence_store.save_success(data=self.data, result=serialized_response)
301301
except Exception as save_exception:
302302
raise IdempotencyPersistenceLayerError(

aws_lambda_powertools/utilities/idempotency/persistence/redis.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -332,8 +332,8 @@ def _item_to_data_record(self, idempotency_key: str, item: dict[str, Any]) -> Da
332332
idempotency_key=idempotency_key,
333333
status=item[self.status_attr],
334334
in_progress_expiry_timestamp=in_progress_expiry_timestamp,
335-
response_data=str(item.get(self.data_attr)),
336-
payload_hash=str(item.get(self.validation_key_attr)),
335+
response_data=item.get(self.data_attr, ""),
336+
payload_hash=item.get(self.validation_key_attr, ""),
337337
expiry_timestamp=item.get("expiration"),
338338
)
339339

tests/functional/idempotency/_redis/test_redis_layer.py

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,25 @@ def test_item_to_datarecord_conversion(valid_record):
330330
assert record.in_progress_expiry_timestamp == item[layer.in_progress_expiry_attr]
331331

332332

333+
def test_item_to_datarecord_conversion_missing_optional_attributes(persistence_store_standalone_redis):
334+
"""
335+
When data_attr or validation_key_attr is missing from Redis,
336+
response_data and payload_hash should be empty string, not the string "None".
337+
Regression test for: https://github.com/aws-powertools/powertools-lambda-python/issues/8090
338+
"""
339+
idempotency_key = "test-func#abc123"
340+
item = {
341+
persistence_store_standalone_redis.status_attr: "COMPLETED",
342+
persistence_store_standalone_redis.expiry_attr: 9999999999,
343+
# data_attr and validation_key_attr intentionally absent
344+
}
345+
346+
record = persistence_store_standalone_redis._item_to_data_record(idempotency_key, item)
347+
348+
assert record.response_data == ""
349+
assert record.payload_hash == ""
350+
351+
333352
def test_idempotent_function_and_lambda_handler_redis_basic(
334353
persistence_store_standalone_redis: RedisCachePersistenceLayer,
335354
lambda_context,

0 commit comments

Comments
 (0)