Skip to content

Commit

Permalink
Disambiguate missing cache values from None
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
swrichards committed Sep 30, 2024
1 parent 08e3b2a commit 00a781b
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 8 deletions.
7 changes: 4 additions & 3 deletions src/open_inwoner/utils/decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
26 changes: 21 additions & 5 deletions src/open_inwoner/utils/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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),
]
)

Expand Down Expand Up @@ -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()

0 comments on commit 00a781b

Please sign in to comment.