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 two tech debt issues found during code review of the idempotency utility.

Changes

persistence/redis.py

• Fixed str(None) producing the string "None" in _item_to_data_record()
  when data_attr or validation_key_attr is missing from Redis.
  Changed str(item.get(...)) to item.get(...) so missing values correctly
  return None instead of the string "None".

persistence/base.py

• Extracted the duplicated 4-line idempotency key null-check (copy-pasted across
  save_success, save_inprogress, delete_record, and get_record) into a
  single reusable helper method _get_idempotency_key_or_return_none().

User experience

Before:

• Functions returning valid falsy values from Redis could silently get "None"
  string stored instead of None
• Same 4-line null-check block duplicated across 4 methods

After:

• Missing Redis attributes correctly return None
• Single helper method handles the null-check cleanly in one place

Notes on remaining items from the issue

After reviewing the current codebase:

• Falsy response handling (if response else None): Does not appear in
  current code — appears already resolved upstream
• STATUS_CONSTANTS consistency: Already using STATUS_CONSTANTS["INPROGRESS"]
  throughout — no change needed

Checklist

• Tests added covering str(None) fix and new helper method
• No breaking changes — purely a bug fix and refactor
• This PR meets the Powertools for AWS Lambda (Python) Tenets
• I have read the CONTRIBUTING guide
• I have followed the Code of Conduct

Acknowledgment

• This PR meets the Powertools for AWS Lambda (Python) Tenets
• I confirm this contribution can be used, modified, copied, and redistributed
  under the terms of your choice.

---

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

[powertools-for-aws-oss-automation]

No acknowledgement section found. Please make sure you used the template to open a PR and didn't remove the acknowledgment section. Check the template at .github/PULL_REQUEST_TEMPLATE.md#acknowledgment

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

[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!