From e0cc7c47e35e817653122fe7c0fc94b2199e033e Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Fri, 26 Feb 2021 19:40:04 -0800 Subject: [PATCH 1/5] feat(idempotency): Fix when delete_record raise a KeyError Fix a KeyError when we an error happens and local_cache is True and the cache is empty --- .../utilities/idempotency/persistence/base.py | 3 ++- tests/functional/idempotency/test_idempotency.py | 11 +++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/aws_lambda_powertools/utilities/idempotency/persistence/base.py b/aws_lambda_powertools/utilities/idempotency/persistence/base.py index c3183e0df84..7e31c7d394b 100644 --- a/aws_lambda_powertools/utilities/idempotency/persistence/base.py +++ b/aws_lambda_powertools/utilities/idempotency/persistence/base.py @@ -272,7 +272,8 @@ def _retrieve_from_cache(self, idempotency_key: str): def _delete_from_cache(self, idempotency_key: str): if not self.use_local_cache: return - del self._cache[idempotency_key] + if idempotency_key in self._cache: + del self._cache[idempotency_key] def save_success(self, event: Dict[str, Any], result: dict) -> None: """ diff --git a/tests/functional/idempotency/test_idempotency.py b/tests/functional/idempotency/test_idempotency.py index 269ab6f9b33..6a85d69d957 100644 --- a/tests/functional/idempotency/test_idempotency.py +++ b/tests/functional/idempotency/test_idempotency.py @@ -627,3 +627,14 @@ def test_user_local_disabled(persistence_store): # THEN raise AttributeError # AND don't have a _cache attribute assert not hasattr("persistence_store", "_cache") + + +@pytest.mark.parametrize("persistence_store", [{"use_local_cache": True}], indirect=True) +def test_delete_from_cache_when_empty(persistence_store): + # GIVEN use_local_cache is True AND the local cache is empty + try: + # WHEN we _delete_from_cache + persistence_store._delete_from_cache("key_does_not_exist") + except KeyError: + # THEN we should not get a KeyError + pytest.fail("KeyError should not happen") From 303a44d30fe21fe78e4e97fb1d9f99e12c8ee96d Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Sat, 27 Feb 2021 19:20:25 -0800 Subject: [PATCH 2/5] tests(LRUDict): Add failing test --- tests/unit/test_lru_cache.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/unit/test_lru_cache.py b/tests/unit/test_lru_cache.py index 170972432ce..b48361c4cb7 100644 --- a/tests/unit/test_lru_cache.py +++ b/tests/unit/test_lru_cache.py @@ -56,3 +56,14 @@ def test_setitem_moves_to_end(populated_cache): 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 From 9a0615d1894d475d278b2b48d6310684f1e731b8 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Sun, 28 Feb 2021 00:21:50 -0800 Subject: [PATCH 3/5] tests(LRUDict): Add passing del test --- tests/unit/test_lru_cache.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/unit/test_lru_cache.py b/tests/unit/test_lru_cache.py index b48361c4cb7..887e20d6270 100644 --- a/tests/unit/test_lru_cache.py +++ b/tests/unit/test_lru_cache.py @@ -67,3 +67,14 @@ def test_lru_pop_failing(): 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 From 608660e79110e5bc09632f01672293a08c952347 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Sun, 28 Feb 2021 08:39:48 -0800 Subject: [PATCH 4/5] 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 --- aws_lambda_powertools/shared/cache_dict.py | 37 ++++++------ .../utilities/idempotency/persistence/base.py | 3 +- tests/unit/test_lru_cache.py | 60 ++++++++----------- 3 files changed, 44 insertions(+), 56 deletions(-) diff --git a/aws_lambda_powertools/shared/cache_dict.py b/aws_lambda_powertools/shared/cache_dict.py index d7184cc1e2b..a58224f58f4 100644 --- a/aws_lambda_powertools/shared/cache_dict.py +++ b/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 --git a/aws_lambda_powertools/utilities/idempotency/persistence/base.py b/aws_lambda_powertools/utilities/idempotency/persistence/base.py index 7e31c7d394b..1a7109fc4fe 100644 --- a/aws_lambda_powertools/utilities/idempotency/persistence/base.py +++ b/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 --git a/tests/unit/test_lru_cache.py b/tests/unit/test_lru_cache.py index 887e20d6270..5be1f15d203 100644 --- a/tests/unit/test_lru_cache.py +++ b/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}" From 4eaffd91466304d574ae230b2d98f66ef7355c96 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Sun, 28 Feb 2021 10:16:04 -0800 Subject: [PATCH 5/5] chore: Revert to original fix --- aws_lambda_powertools/shared/cache_dict.py | 37 ++++++------ .../utilities/idempotency/persistence/base.py | 3 +- tests/unit/test_lru_cache.py | 60 +++++++++++-------- 3 files changed, 56 insertions(+), 44 deletions(-) diff --git a/aws_lambda_powertools/shared/cache_dict.py b/aws_lambda_powertools/shared/cache_dict.py index a58224f58f4..d7184cc1e2b 100644 --- a/aws_lambda_powertools/shared/cache_dict.py +++ b/aws_lambda_powertools/shared/cache_dict.py @@ -1,32 +1,31 @@ from collections import OrderedDict -class LRUDict: +class LRUDict(OrderedDict): """ Cache implementation based on ordered dict with a maximum number of items. Last accessed item will be evicted - first, unless the item is None which will be evicted first. Currently used only by idempotency utility. + first. Currently used by idempotency utility. """ def __init__(self, max_items=1024, *args, **kwargs): self.max_items = max_items - self._cache = OrderedDict(*args, **kwargs) + super().__init__(*args, **kwargs) - def __getitem__(self, key: str): - value = self._cache.__getitem__(key) - if value: - self._cache.move_to_end(key) + def __getitem__(self, key): + value = super().__getitem__(key) + self.move_to_end(key) return value - 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 __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 get(self, key: str): - value = self._cache.get(key) - if value: - self._cache.move_to_end(key) - return value + def get(self, key, *args, **kwargs): + item = super(LRUDict, self).get(key, *args, **kwargs) + if item: + self.move_to_end(key=key) + return item diff --git a/aws_lambda_powertools/utilities/idempotency/persistence/base.py b/aws_lambda_powertools/utilities/idempotency/persistence/base.py index 1a7109fc4fe..7e31c7d394b 100644 --- a/aws_lambda_powertools/utilities/idempotency/persistence/base.py +++ b/aws_lambda_powertools/utilities/idempotency/persistence/base.py @@ -272,7 +272,8 @@ def _retrieve_from_cache(self, idempotency_key: str): def _delete_from_cache(self, idempotency_key: str): if not self.use_local_cache: return - self._cache[idempotency_key] = None + if idempotency_key in self._cache: + del self._cache[idempotency_key] def save_success(self, event: Dict[str, Any], result: dict) -> None: """ diff --git a/tests/unit/test_lru_cache.py b/tests/unit/test_lru_cache.py index 5be1f15d203..887e20d6270 100644 --- a/tests/unit/test_lru_cache.py +++ b/tests/unit/test_lru_cache.py @@ -9,60 +9,72 @@ @pytest.fixture -def populated_cache() -> LRUDict: +def populated_cache(): 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: LRUDict): - first_item = list(populated_cache._cache)[0] - last_item = list(populated_cache._cache)[-1] +def test_cache_order_init(populated_cache): + first_item = list(populated_cache)[0] + last_item = list(populated_cache)[-1] assert first_item == "key_0" assert last_item == f"key_{MAX_CACHE_ITEMS - 1}" -def test_cache_order_getitem(populated_cache: LRUDict): +def test_cache_order_getitem(populated_cache): random_value = random.randrange(0, MAX_CACHE_ITEMS) _ = populated_cache[f"key_{random_value}"] - last_item = list(populated_cache._cache)[-1] + last_item = list(populated_cache)[-1] assert last_item == f"key_{random_value}" -def test_cache_order_get(populated_cache: LRUDict): +def test_cache_order_get(populated_cache): random_value = random.randrange(0, MAX_CACHE_ITEMS) _ = populated_cache.get(f"key_{random_value}") - last_item = list(populated_cache._cache)[-1] + last_item = list(populated_cache)[-1] assert last_item == f"key_{random_value}" -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 +def test_cache_evict_over_max_items(populated_cache): + assert "key_0" in populated_cache + assert len(populated_cache) == MAX_CACHE_ITEMS populated_cache["new_item"] = "new_value" - assert len(populated_cache._cache) == MAX_CACHE_ITEMS - assert "key_0" not in populated_cache._cache - assert "key_1" in populated_cache._cache + assert len(populated_cache) == MAX_CACHE_ITEMS + assert "key_0" not in populated_cache + assert "key_1" in populated_cache -def test_setitem_moves_to_end(populated_cache: LRUDict): +def test_setitem_moves_to_end(populated_cache): random_value = random.randrange(0, MAX_CACHE_ITEMS) populated_cache[f"key_{random_value}"] = f"new_val_{random_value}" - last_item = list(populated_cache._cache)[-1] + last_item = list(populated_cache)[-1] assert last_item == f"key_{random_value}" assert populated_cache[f"key_{random_value}"] == f"new_val_{random_value}" -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}" +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