Merge pull request #25 from ycharts/feature/cache-key-consistency-improvements

Improve consistency of cache key generation with args/kwargs

Author: nbecker-yc

cache_helper/decorators.py

@@ -1,4 +1,6 @@
 import logging
+from inspect import Signature, signature
+from typing import Tuple  # deprecated, but required for Python 3.8 and below
 
 try:
     from _pylibmc import Error as CacheSetError
@@ -15,23 +17,41 @@
 logger = logging.getLogger(__name__)
 
 
+def _get_function_cache_keys(func_name: str, func_signature: Signature, args: tuple, kwargs: dict) -> Tuple[str, str]:
+    """
+    Generate hashed and non-hashed function cache keys, ensuring that args and kwargs are correctly bound to function.
+
+    :param func_name: The fully specified name of the function to be cached.
+    :param func_signature: The signature of the function to be cached.
+    :param args: The positional arguments passed to the function.
+    :param kwargs: The keyword arguments passed to the function.
+
+    :return: A tuple containing the hashed cache key and the non-hashed cache key.
+    """
+    bound_arguments = func_signature.bind(*args, **kwargs)
+    bound_arguments.apply_defaults()
+    cache_key_string = utils.get_function_cache_key(func_name, bound_arguments.args, bound_arguments.kwargs)
+    cache_key_hashed = utils.get_hashed_cache_key(cache_key_string)
+    return cache_key_hashed, cache_key_string
+
+
 def cached(timeout):
     def _cached(func):
         func_name = utils.get_function_name(func)
+        func_signature = signature(func)
 
         @wraps(func)
         def wrapper(*args, **kwargs):
-            function_cache_key = utils.get_function_cache_key(func_name, args, kwargs)
-            cache_key = utils.get_hashed_cache_key(function_cache_key)
+            cache_key_hashed, cache_key_string = _get_function_cache_keys(func_name, func_signature, args, kwargs)
 
             # We need to determine whether the object exists in the cache, and since we may have stored a literal value
             # None, use a sentinel object as the default
             sentinel = object()
             try:
-                value = cache.get(cache_key, sentinel)
+                value = cache.get(cache_key_hashed, sentinel)
             except Exception:
                 logger.warning(
-                    f'Error retrieving value from Cache for Key: {function_cache_key}',
+                    f'Error retrieving value from Cache for Key: {cache_key_string}',
                     exc_info=True,
                 )
                 value = sentinel
@@ -41,7 +61,7 @@ def wrapper(*args, **kwargs):
             if value is None:
                 logger.warning(
                     'None cache value found for cache key: {}, function cache key: {}, value: {}'.format(
-                        cache_key, function_cache_key, value
+                        cache_key_hashed, cache_key_string, value
                     )
                 )
 
@@ -51,10 +71,10 @@ def wrapper(*args, **kwargs):
                 # But if it fails on an error from the underlying
                 # cache system, handle it.
                 try:
-                    cache.set(cache_key, value, timeout)
+                    cache.set(cache_key_hashed, value, timeout)
                 except CacheSetError:
                     logger.warning(
-                        f'Error saving value to Cache for Key: {function_cache_key}',
+                        f'Error saving value to Cache for Key: {cache_key_string}',
                         exc_info=True,
                     )
 
@@ -68,8 +88,7 @@ def invalidate(*args, **kwargs):
             :param kwargs: The kwargs passed into the original function.
             :rtype: None
             """
-            function_cache_key = utils.get_function_cache_key(func_name, args, kwargs)
-            cache_key = utils.get_hashed_cache_key(function_cache_key)
+            cache_key, _ = _get_function_cache_keys(func_name, func_signature, args, kwargs)
             cache.delete(cache_key)
 
         wrapper.invalidate = invalidate
@@ -81,21 +100,23 @@ def invalidate(*args, **kwargs):
 def cached_class_method(timeout):
     def _cached(func):
         func_name = utils.get_function_name(func)
+        func_signature = signature(func)
 
         @wraps(func)
         def wrapper(*args, **kwargs):
-            # skip the first arg because it will be the class itself
-            function_cache_key = utils.get_function_cache_key(func_name, args[1:], kwargs)
-            cache_key = utils.get_hashed_cache_key(function_cache_key)
-
+            # replace the first arg for caching purposes because it will be the class itself
+            cls_adjusted_args = (None, *args[1:])
+            cache_key_hashed, cache_key_string = _get_function_cache_keys(
+                func_name, func_signature, cls_adjusted_args, kwargs
+            )
             # We need to determine whether the object exists in the cache, and since we may have stored a literal value
             # None, use a sentinel object as the default
             sentinel = object()
             try:
-                value = cache.get(cache_key, sentinel)
+                value = cache.get(cache_key_hashed, sentinel)
             except Exception:
                 logger.warning(
-                    f'Error retrieving value from Cache for Key: {function_cache_key}',
+                    f'Error retrieving value from Cache for Key: {cache_key_string}',
                     exc_info=True,
                 )
                 value = sentinel
@@ -105,7 +126,7 @@ def wrapper(*args, **kwargs):
             if value is None:
                 logger.warning(
                     'None cache value found for cache key: {}, function cache key: {}, value: {}'.format(
-                        cache_key, function_cache_key, value
+                        cache_key_hashed, cache_key_string, value
                     )
                 )
 
@@ -115,10 +136,10 @@ def wrapper(*args, **kwargs):
                 # But if it fails on an error from the underlying
                 # cache system, handle it.
                 try:
-                    cache.set(cache_key, value, timeout)
+                    cache.set(cache_key_hashed, value, timeout)
                 except CacheSetError:
                     logger.warning(
-                        f'Error saving value to Cache for Key: {function_cache_key}',
+                        f'Error saving value to Cache for Key: {cache_key_string}',
                         exc_info=True,
                     )
 
@@ -127,13 +148,16 @@ def wrapper(*args, **kwargs):
         def invalidate(*args, **kwargs):
             """
             A method to invalidate a result from the cache.
-            :param args: The args passed into the original function. This includes `self` for instance methods, and
+            :param args: The args passed into the original function. This excludes `self` for instance methods, and
             `cls` for class methods.
             :param kwargs: The kwargs passed into the original function.
             :rtype: None
             """
-            function_cache_key = utils.get_function_cache_key(func_name, args, kwargs)
-            cache_key = utils.get_hashed_cache_key(function_cache_key)
+            # note: args does not include the class itself, but because it *is* passed to wrapper() and we replaced
+            # it with None for consistent cache behavior with subclasses, we need to account for it here by updating
+            # args to include None
+            cls_adjusted_args = (None, *args)
+            cache_key, _ = _get_function_cache_keys(func_name, func_signature, cls_adjusted_args, kwargs)
             cache.delete(cache_key)
 
         wrapper.invalidate = invalidate
@@ -171,16 +195,16 @@ def __get__(self, obj, objtype):
             return fn
 
         def __call__(self, *args, **kwargs):
-            cache_key, function_cache_key = self.create_cache_key(*args, **kwargs)
+            cache_key_hashed, cache_key_string = self.create_cache_key(*args, **kwargs)
 
             # We need to determine whether the object exists in the cache, and since we may have stored a literal value
             # None, use a sentinel object as the default
             sentinel = object()
             try:
-                value = cache.get(cache_key, sentinel)
+                value = cache.get(cache_key_hashed, sentinel)
             except Exception:
                 logger.warning(
-                    f'Error retrieving value from Cache for Key: {function_cache_key}',
+                    f'Error retrieving value from Cache for Key: {cache_key_string}',
                     exc_info=True,
                 )
                 value = sentinel
@@ -190,7 +214,7 @@ def __call__(self, *args, **kwargs):
             if value is None:
                 logger.warning(
                     'None cache value found for cache key: {}, function cache key: {}, value: {}'.format(
-                        cache_key, function_cache_key, value
+                        cache_key_hashed, cache_key_string, value
                     )
                 )
 
@@ -200,10 +224,10 @@ def __call__(self, *args, **kwargs):
                 # But if it fails on an error from the underlying
                 # cache system, handle it.
                 try:
-                    cache.set(cache_key, value, timeout)
+                    cache.set(cache_key_hashed, value, timeout)
                 except CacheSetError:
                     logger.warning(
-                        f'Error saving value to Cache for Key: {function_cache_key}',
+                        f'Error saving value to Cache for Key: {cache_key_string}',
                         exc_info=True,
                     )
 
@@ -217,14 +241,14 @@ def _invalidate(self, *args, **kwargs):
             :param kwargs: The kwargs passed into the original function.
             :rtype: None
             """
-            cache_key, _ = self.create_cache_key(*args, **kwargs)
-            cache.delete(cache_key)
+            cache_key_hashed, _ = self.create_cache_key(*args, **kwargs)
+            cache.delete(cache_key_hashed)
 
         def create_cache_key(self, *args, **kwargs):
             # Need to include the first arg (self) in the cache key
             func_name = utils.get_function_name(self.func)
-            function_cache_key = utils.get_function_cache_key(func_name, args, kwargs)
-            cache_key = utils.get_hashed_cache_key(function_cache_key)
-            return cache_key, function_cache_key
+            func_signature = signature(self.func)
+            cache_key_hashed, cache_key_string = _get_function_cache_keys(func_name, func_signature, args, kwargs)
+            return cache_key_hashed, cache_key_string
 
     return wrapper