From 00a781be2595c5e97a40210e429bc1babb70bce7 Mon Sep 17 00:00:00 2001 From: Sidney Richards Date: Thu, 26 Sep 2024 15:16:12 +0200 Subject: [PATCH] Disambiguate missing cache values from None We have observed an issue with spurious cache hits in development, that we've been unable to reproduce so far. Looking into this did however prompt an audit of the caching decorator, which reveals we potentially have an issue where a cached "None" value is mis- interpreted as "not cached" rather than "cached as None". This commit clarifies the expected behaviour with a test and a more robust cache check. --- src/open_inwoner/utils/decorators.py | 7 +++--- src/open_inwoner/utils/tests/test_utils.py | 26 +++++++++++++++++----- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/open_inwoner/utils/decorators.py b/src/open_inwoner/utils/decorators.py index 8684113917..593787e913 100644 --- a/src/open_inwoner/utils/decorators.py +++ b/src/open_inwoner/utils/decorators.py @@ -120,11 +120,12 @@ def wrapped(*args, **kwargs) -> RT: cache_key = cache_key_with_attr_placeholders.format(**key_kwargs) logger.debug("Resolved cache_key `%s` to `%s`", key, cache_key) + _cache: BaseCache = caches[alias] - result = _cache.get(cache_key) - # The key exists in cache so we return the already cached data - if result is not None: + CACHE_MISS = object() + result = _cache.get(cache_key, default=CACHE_MISS) + if result is not CACHE_MISS: logger.debug("Cache hit: '%s'", cache_key) return result diff --git a/src/open_inwoner/utils/tests/test_utils.py b/src/open_inwoner/utils/tests/test_utils.py index afc4c4ea48..36d565e8e4 100644 --- a/src/open_inwoner/utils/tests/test_utils.py +++ b/src/open_inwoner/utils/tests/test_utils.py @@ -55,11 +55,11 @@ def with_static_key(self): self.cache.get.assert_has_calls( [ - mock.call("alpha:charlie:bravo:42"), - mock.call("alpha:bar:bravo:baz"), - mock.call("alpha:bar:bravo:5"), - mock.call("alpha:bar:bravo:baz:charlie:charlie:42"), - mock.call("static"), + mock.call("alpha:charlie:bravo:42", default=mock.ANY), + mock.call("alpha:bar:bravo:baz", default=mock.ANY), + mock.call("alpha:bar:bravo:5", default=mock.ANY), + mock.call("alpha:bar:bravo:baz:charlie:charlie:42", default=mock.ANY), + mock.call("static", default=mock.ANY), ] ) @@ -201,3 +201,19 @@ def foo(): with self.assertRaises(ValueError): foo() + + def test_returning_None_is_not_treated_as_a_cache_miss(self): + m = mock.Mock() + + @cache("foo") + def returns_none(): + m() + return None + + # The second call should return the cached "None" from the first call, + # which the cache decorator should interpret as a valid cached value, + # not as a cache miss. + returns_none() + returns_none() + + m.assert_called_once()