chore: move cache conditionals inside of cache methods

Author: Tom McCarthy

aws_lambda_powertools/utilities/idempotency/persistence/base.py

@@ -260,6 +260,8 @@ def _save_to_cache(self, data_record: DataRecord):
         self._cache[data_record.idempotency_key] = data_record
 
     def _retrieve_from_cache(self, idempotency_key: str):
+        if not self.use_local_cache:
+            return
         cached_record = self._cache.get(idempotency_key)
         if cached_record:
             if not cached_record.is_expired:
@@ -268,6 +270,8 @@ def _retrieve_from_cache(self, idempotency_key: str):
             self._delete_from_cache(idempotency_key)
 
     def _delete_from_cache(self, idempotency_key: str):
+        if not self.use_local_cache:
+            return
         del self._cache[idempotency_key]
 
     def save_success(self, event: Dict[str, Any], result: dict) -> None:
@@ -296,8 +300,7 @@ def save_success(self, event: Dict[str, Any], result: dict) -> None:
         )
         self._update_record(data_record=data_record)
 
-        if self.use_local_cache:
-            self._save_to_cache(data_record)
+        self._save_to_cache(data_record)
 
     def save_inprogress(self, event: Dict[str, Any]) -> None:
         """
@@ -317,10 +320,8 @@ def save_inprogress(self, event: Dict[str, Any]) -> None:
 
         logger.debug(f"Saving in progress record for idempotency key: {data_record.idempotency_key}")
 
-        if self.use_local_cache:
-            cached_record = self._retrieve_from_cache(idempotency_key=data_record.idempotency_key)
-            if cached_record:
-                raise IdempotencyItemAlreadyExistsError
+        if self._retrieve_from_cache(idempotency_key=data_record.idempotency_key):
+            raise IdempotencyItemAlreadyExistsError
 
         self._put_record(data_record)
 
@@ -343,8 +344,7 @@ def delete_record(self, event: Dict[str, Any], exception: Exception):
         )
         self._delete_record(data_record)
 
-        if self.use_local_cache:
-            self._delete_from_cache(data_record.idempotency_key)
+        self._delete_from_cache(data_record.idempotency_key)
 
     def get_record(self, event: Dict[str, Any]) -> DataRecord:
         """
@@ -370,19 +370,15 @@ def get_record(self, event: Dict[str, Any]) -> DataRecord:
 
         idempotency_key = self._get_hashed_idempotency_key(event)
 
-        if self.use_local_cache:
-            cached_record = self._retrieve_from_cache(idempotency_key=idempotency_key)
-            if cached_record:
-                logger.debug(f"Idempotency record found in cache with idempotency key: {idempotency_key}")
-                self._validate_payload(event, cached_record)
-                return cached_record
+        cached_record = self._retrieve_from_cache(idempotency_key=idempotency_key)
+        if cached_record:
+            logger.debug(f"Idempotency record found in cache with idempotency key: {idempotency_key}")
+            self._validate_payload(event, cached_record)
+            return cached_record
 
         record = self._get_record(idempotency_key)
 
-        # We can't cache "INPROGRESS" records as we have no way to reflect updates that can happen outside of the
-        # execution environment
-        if self.use_local_cache and not record.status == STATUS_CONSTANTS["INPROGRESS"]:
-            self._save_to_cache(data_record=record)
+        self._save_to_cache(data_record=record)
 
         self._validate_payload(event, record)
         return record

tests/functional/idempotency/test_idempotency.py

@@ -157,7 +157,7 @@ def lambda_handler(event, context):
     assert retrieve_from_cache_spy.call_count == 2 * loops
     retrieve_from_cache_spy.assert_called_with(idempotency_key=hashed_idempotency_key)
 
-    save_to_cache_spy.assert_not_called()
+    save_to_cache_spy.assert_called()
     assert persistence_store._cache.get(hashed_idempotency_key) is None
 
     stubber.assert_no_pending_responses()
@@ -595,3 +595,35 @@ def test_data_record_invalid_status_value():
         _ = data_record.status
 
     assert e.value.args[0] == "UNSUPPORTED_STATUS"
+
+
+@pytest.mark.parametrize("persistence_store", [{"use_local_cache": True}], indirect=True)
+def test_in_progress_never_saved_to_cache(persistence_store):
+    # GIVEN a data record with status "INPROGRESS"
+    # and persistence_store has use_local_cache = True
+    data_record = DataRecord("key", status="INPROGRESS")
+
+    # WHEN saving to local cache
+    persistence_store._save_to_cache(data_record)
+
+    # THEN don't save to local cache
+    assert persistence_store._cache.get("key") is None
+
+
+@pytest.mark.parametrize("persistence_store", [{"use_local_cache": False}], indirect=True)
+def test_user_local_disabled(persistence_store):
+    # GIVEN a persistence_store with use_local_cache = False
+
+    # WHEN calling any local cache options
+    data_record = DataRecord("key", status="COMPLETED")
+    try:
+        persistence_store._save_to_cache(data_record)
+        cache_value = persistence_store._retrieve_from_cache("key")
+        assert cache_value is None
+        persistence_store._delete_from_cache("key")
+    except AttributeError as e:
+        pytest.fail(f"AttributeError should not be raised: {e}")
+
+    # THEN raise AttributeError
+    # AND don't have a _cache attribute
+    assert not hasattr("persistence_store", "_cache")