fix(idempotency): fix str(None) in Redis persistence and extract duplicated null-check helper - tech debt 8090

[hirenkumar-n-dholariya]

Issue number: fixes #8090

Summary

Fixes all 4 tech debt issues found during code review of the idempotency utility.

Changes

aws_lambda_powertools/utilities/idempotency/base.py

• Fix falsy response handling: if response else None → if response is not None else None
  so valid falsy return values (0, False, {}, [], "") are correctly serialized and stored instead of being silently discarded on replay
• Fix inconsistent status constant: string literal "INPROGRESS" →
  STATUS_CONSTANTS["INPROGRESS"] for consistency with the rest of the codebase

aws_lambda_powertools/utilities/idempotency/persistence/redis.py

• Fix str(None) producing the string "None" in _item_to_data_record() when data_attr or validation_key_attr is missing from Redis

User experience

Before: Functions returning falsy values (0, False, {}) would have their response silently discarded and re-executed on replay instead of returning the cached value.

After: All non-None return values including falsy ones are correctly cached and returned on replay.

Tests

Added one regression test in tests/functional/idempotency/_redis/test_redis_layer.py next to existing test_item_to_datarecord_conversion, verifying missing Redis attributes return None instead of the string "None".

Acknowledgment

• This PR meets the [Powertools for AWS Lambda (Python) Tenets] https://docs.powertools.aws.dev/lambda/python/latest/#tenets)
• I have read the CONTRIBUTING guide
• I have followed the Code of Conduct

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

[leandrodamascena]

Hey @hirenkumar-n-dholariya, thanks for working on this and submitting the PR!

The Redis str(None) fix is good. Removing the str() wrapper is the right fix.

However, I have some concerns with the rest of the PR, and there are missing items from the issue.

Missing items from the issue

In your comment on #8090 you mentioned that items 1 and 2 "appear already resolved", but they are still present in the codebase:

Item 1 - Falsy response handling is still in base.py:299:

serialized_response: dict = self.output_serializer.to_dict(response) if response else None

This silently discards valid falsy return values like 0, False, "", [], {}. It should be if response is not None. This is a real bug that affects customers.

Item 2 - "INPROGRESS" string literal is still in base.py:170:

if is_replay and record is not None and record.status == "INPROGRESS":

Should use STATUS_CONSTANTS["INPROGRESS"] for consistency with the rest of the code.

Helper method

The helper _get_idempotency_key_or_return_none() doesn't actually reduce the duplication. After the refactor, each caller still does:

idempotency_key = self._get_idempotency_key_or_return_none(data=data)
if idempotency_key is None:
    return None

We moved one line into a wrapper but the if None: return is still in all 4 methods. The helper is a passthrough that adds indirection without simplifying anything. Let's drop this part and keep the original inline pattern, it's 3 lines and every Python developer reads it instantly.

Tests

The tests need a full rework. Please don't create a new test file for this.

We already have test_item_to_datarecord_conversion in tests/functional/idempotency/_redis/test_redis_layer.py (line 318) that tests exactly _item_to_data_record(). Add your test case there, next to the existing one. That's our pattern: tests go in the existing test files, close to related tests.

What's wrong with the current tests:

• Don't test Python builtins. test_str_none_produces_wrong_string asserts that str(None) == "None" and test_redis_missing_data_attr_returns_none_not_string asserts that dict.get() returns None. These are testing the Python language, not our code.
• Don't mock internal methods with MagicMock. We don't do that in idempotency tests. Look at the existing tests in _redis/test_redis_layer.py and _boto3/test_idempotency.py for the patterns we use. We test through the actual classes with fixtures, not by patching _get_hashed_idempotency_key with a MagicMock.
• ConcretePersistenceLayer is defined 5 times instead of once.
• 208 lines of tests for a 2-line fix is too much surface area. A single test in the existing file that calls _item_to_data_record() with missing attributes and asserts response_data is None (not "None") would cover the Redis fix properly.

Suggestion

Let's scope this PR to fix all 4 items from the issue:

1. Fix if response else None to if response is not None (item 1 - bug fix)
2. Fix "INPROGRESS" string literal to use STATUS_CONSTANTS (item 2 - consistency)
3. Fix str(None) in Redis (item 3 - already done, good)
4. Drop the helper method extraction (item 4 - the current approach doesn't improve things)

Add tests in the existing test files following our current patterns. Also pls fix the issues in SonarCloud.

Thanks again for contributing!

[hirenkumar-n-dholariya]

Hey @hirenkumar-n-dholariya, thanks for working on this and submitting the PR!

The Redis str(None) fix is good. Removing the str() wrapper is the right fix.

However, I have some concerns with the rest of the PR, and there are missing items from the issue.

Missing items from the issue

In your comment on #8090 you mentioned that items 1 and 2 "appear already resolved", but they are still present in the codebase:

Item 1 - Falsy response handling is still in base.py:299:

serialized_response: dict = self.output_serializer.to_dict(response) if response else None

This silently discards valid falsy return values like 0, False, "", [], {}. It should be if response is not None. This is a real bug that affects customers.

Item 2 - "INPROGRESS" string literal is still in base.py:170:

if is_replay and record is not None and record.status == "INPROGRESS":

Should use STATUS_CONSTANTS["INPROGRESS"] for consistency with the rest of the code.

Helper method

The helper _get_idempotency_key_or_return_none() doesn't actually reduce the duplication. After the refactor, each caller still does:

idempotency_key = self._get_idempotency_key_or_return_none(data=data)
if idempotency_key is None:
    return None

We moved one line into a wrapper but the if None: return is still in all 4 methods. The helper is a passthrough that adds indirection without simplifying anything. Let's drop this part and keep the original inline pattern, it's 3 lines and every Python developer reads it instantly.

Tests

The tests need a full rework. Please don't create a new test file for this.

We already have test_item_to_datarecord_conversion in tests/functional/idempotency/_redis/test_redis_layer.py (line 318) that tests exactly _item_to_data_record(). Add your test case there, next to the existing one. That's our pattern: tests go in the existing test files, close to related tests.

What's wrong with the current tests:

• Don't test Python builtins. test_str_none_produces_wrong_string asserts that str(None) == "None" and test_redis_missing_data_attr_returns_none_not_string asserts that dict.get() returns None. These are testing the Python language, not our code.
• Don't mock internal methods with MagicMock. We don't do that in idempotency tests. Look at the existing tests in _redis/test_redis_layer.py and _boto3/test_idempotency.py for the patterns we use. We test through the actual classes with fixtures, not by patching _get_hashed_idempotency_key with a MagicMock.
• ConcretePersistenceLayer is defined 5 times instead of once.
• 208 lines of tests for a 2-line fix is too much surface area. A single test in the existing file that calls _item_to_data_record() with missing attributes and asserts response_data is None (not "None") would cover the Redis fix properly.

Suggestion

Let's scope this PR to fix all 4 items from the issue:

1. Fix if response else None to if response is not None (item 1 - bug fix)
2. Fix "INPROGRESS" string literal to use STATUS_CONSTANTS (item 2 - consistency)
3. Fix str(None) in Redis (item 3 - already done, good)
4. Drop the helper method extraction (item 4 - the current approach doesn't improve things)

Add tests in the existing test files following our current patterns. Also pls fix the issues in SonarCloud.

Thanks again for contributing!

Hey @leandrodamascena,
Thank you for the thorough and detailed review - really appreciate the clear guidance!

I have addressed all the feedback:

1. Fixed falsy response handling - I changed if response else None to if response is not None else None in idempotency/base.py so valid falsy return values (0, False, {}, [], "") are correctly cached and returned on replay

2. Fixed STATUS_CONSTANTS consistency - I replaced string literal "INPROGRESS" with STATUS_CONSTANTS["INPROGRESS"] in_process_idempotency()`

3. Reverted helper method - restored the original inline 3-line null-check pattern across all 4 methods in persistence/base.py. You are right that the helper added indirection without real simplification.

4. Tests reworked - removed the standalone test file and added a single focused test directly in _redis/test_redis_layer.py next to test_item_to_datarecord_conversion, following the existing fixture-based patterns

5. Fixed PR description - added the Acknowledgment section as required by the template

Thanks again for taking the time to explain the patterns and reasoning behind each point. It really helps understand the project's conventions better. Please let me know if anything else needs adjusting.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud