refactor(LRUDict): Setting key to None moves to beginning

Alternative fix by just exposing get, __getitem__ and __setitem__ and when setting to None moves the item to the beginning

Author: Michael Brewer

Diff for: aws_lambda_powertools/shared/cache_dict.py

@@ -1,31 +1,32 @@
 from collections import OrderedDict
 
 
-class LRUDict(OrderedDict):
+class LRUDict:
     """
     Cache implementation based on ordered dict with a maximum number of items. Last accessed item will be evicted
-    first. Currently used by idempotency utility.
+    first, unless the item is None which will be evicted first. Currently used only by idempotency utility.
     """
 
     def __init__(self, max_items=1024, *args, **kwargs):
         self.max_items = max_items
-        super().__init__(*args, **kwargs)
+        self._cache = OrderedDict(*args, **kwargs)
 
-    def __getitem__(self, key):
-        value = super().__getitem__(key)
-        self.move_to_end(key)
+    def __getitem__(self, key: str):
+        value = self._cache.__getitem__(key)
+        if value:
+            self._cache.move_to_end(key)
         return value
 
-    def __setitem__(self, key, value):
-        if key in self:
-            self.move_to_end(key)
-        super().__setitem__(key, value)
-        if len(self) > self.max_items:
-            oldest = next(iter(self))
-            del self[oldest]
+    def __setitem__(self, key: str, value):
+        self._cache.__setitem__(key, value)
+        if key in self._cache:
+            self._cache.move_to_end(key, last=value is not None)
+        if len(self._cache) > self.max_items:
+            oldest = next(iter(self._cache))
+            del self._cache[oldest]
 
-    def get(self, key, *args, **kwargs):
-        item = super(LRUDict, self).get(key, *args, **kwargs)
-        if item:
-            self.move_to_end(key=key)
-        return item
+    def get(self, key: str):
+        value = self._cache.get(key)
+        if value:
+            self._cache.move_to_end(key)
+        return value

Diff for: aws_lambda_powertools/utilities/idempotency/persistence/base.py

@@ -272,8 +272,7 @@ def _retrieve_from_cache(self, idempotency_key: str):
     def _delete_from_cache(self, idempotency_key: str):
         if not self.use_local_cache:
             return
-        if idempotency_key in self._cache:
-            del self._cache[idempotency_key]
+        self._cache[idempotency_key] = None
 
     def save_success(self, event: Dict[str, Any], result: dict) -> None:
         """

Diff for: tests/unit/test_lru_cache.py

@@ -9,72 +9,60 @@
 
 
 @pytest.fixture
-def populated_cache():
+def populated_cache() -> LRUDict:
     cache_dict = LRUDict(max_items=MAX_CACHE_ITEMS, **{f"key_{i}": f"val_{i}" for i in range(0, PREFILL_CACHE_ITEMS)})
     return cache_dict
 
 
-def test_cache_order_init(populated_cache):
-    first_item = list(populated_cache)[0]
-    last_item = list(populated_cache)[-1]
+def test_cache_order_init(populated_cache: LRUDict):
+    first_item = list(populated_cache._cache)[0]
+    last_item = list(populated_cache._cache)[-1]
 
     assert first_item == "key_0"
     assert last_item == f"key_{MAX_CACHE_ITEMS - 1}"
 
 
-def test_cache_order_getitem(populated_cache):
+def test_cache_order_getitem(populated_cache: LRUDict):
     random_value = random.randrange(0, MAX_CACHE_ITEMS)
     _ = populated_cache[f"key_{random_value}"]
 
-    last_item = list(populated_cache)[-1]
+    last_item = list(populated_cache._cache)[-1]
 
     assert last_item == f"key_{random_value}"
 
 
-def test_cache_order_get(populated_cache):
+def test_cache_order_get(populated_cache: LRUDict):
     random_value = random.randrange(0, MAX_CACHE_ITEMS)
     _ = populated_cache.get(f"key_{random_value}")
 
-    last_item = list(populated_cache)[-1]
+    last_item = list(populated_cache._cache)[-1]
 
     assert last_item == f"key_{random_value}"
 
 
-def test_cache_evict_over_max_items(populated_cache):
-    assert "key_0" in populated_cache
-    assert len(populated_cache) == MAX_CACHE_ITEMS
+def test_cache_evict_over_max_items(populated_cache: LRUDict):
+    assert "key_0" in populated_cache._cache
+    assert len(populated_cache._cache) == MAX_CACHE_ITEMS
     populated_cache["new_item"] = "new_value"
-    assert len(populated_cache) == MAX_CACHE_ITEMS
-    assert "key_0" not in populated_cache
-    assert "key_1" in populated_cache
+    assert len(populated_cache._cache) == MAX_CACHE_ITEMS
+    assert "key_0" not in populated_cache._cache
+    assert "key_1" in populated_cache._cache
 
 
-def test_setitem_moves_to_end(populated_cache):
+def test_setitem_moves_to_end(populated_cache: LRUDict):
     random_value = random.randrange(0, MAX_CACHE_ITEMS)
     populated_cache[f"key_{random_value}"] = f"new_val_{random_value}"
-    last_item = list(populated_cache)[-1]
+    last_item = list(populated_cache._cache)[-1]
 
     assert last_item == f"key_{random_value}"
     assert populated_cache[f"key_{random_value}"] == f"new_val_{random_value}"
 
 
-def test_lru_pop_failing():
-    cache = LRUDict()
-    key = "test"
-    cache[key] = "value"
-    try:
-        cache.pop(key, None)
-        pytest.fail("GitHub #300: LRUDict pop bug has been fixed :)")
-    except KeyError as e:
-        assert e.args[0] == key
-
-
-def test_lru_del():
-    cache = LRUDict()
-    key = "test"
-    cache[key] = "value"
-    assert len(cache) == 1
-    if key in cache:
-        del cache[key]
-    assert key not in cache
-    assert len(cache) == 0
+def test_setitem_none_not_moved(populated_cache: LRUDict):
+    populated_cache["value_is_none"] = None
+
+    first_item = list(populated_cache._cache)[0]
+    last_item = list(populated_cache._cache)[-1]
+
+    assert first_item == "key_0"
+    assert last_item == f"key_{MAX_CACHE_ITEMS - 1}"