Switched imagekit caching from in-memory to redis (#1475)

Author: rhysyngsun

main/cache/backends.py

@@ -1,5 +1,5 @@
 from django.core.cache import caches
-from django.core.cache.backends.base import BaseCache
+from django.core.cache.backends.base import DEFAULT_TIMEOUT, BaseCache
 
 
 class FallbackCache(BaseCache):
@@ -24,20 +24,50 @@ class FallbackCache(BaseCache):
     def __init__(self, cache_names, params):
         super().__init__(params)
         self._cache_names = cache_names
+        self.default_timeout = params.get("TIMEOUT", None)
+
+    def get_backend_timeout(self, timeout=DEFAULT_TIMEOUT):
+        """
+        Return the timeout to pass to fallback caches
+        """
+        if timeout == DEFAULT_TIMEOUT:
+            timeout = self.default_timeout
+
+        return timeout
 
     def get(self, key, default=None, version=None):
         """Get the value from the caches in order"""
+        cache_misses = []
+        result = None
+
         for cache_name in self._cache_names:
             cache = caches[cache_name]
-            result = cache.get(key, default=default, version=version)
-            if result:
-                return result
-        return None
+            # explicitly pass None here because it'd cause a false positive
+            result = cache.get(key, default=None, version=version)
+            if result is not None:
+                break
+            else:
+                cache_misses.append(cache)
+
+        # We need to manually set the value in caches that missed
+        # because consumers of get() will typically not call set() if get()
+        # returns a value. If it doesn't return a value, consumers will
+        # compute the value and then call set().
+        if result is not None:
+            for cache in cache_misses:
+                cache.set(
+                    key, result, timeout=self.get_backend_timeout(), version=version
+                )
+
+            return result
+
+        return default
 
-    def set(self, key, value, timeout=None, version=None):
+    def set(self, key, value, timeout=DEFAULT_TIMEOUT, version=None):
         """Set a value in the caches"""
         for cache_name in self._cache_names:
             cache = caches[cache_name]
+            timeout = self.get_backend_timeout(timeout)
             cache.set(
                 key,
                 value,

main/cache/backends_test.py

@@ -1,44 +1,87 @@
-from django.core.cache.backends.base import BaseCache
+from dataclasses import dataclass
+
+import pytest
+from django.core.cache.backends.base import DEFAULT_TIMEOUT, BaseCache
 
 from main.cache.backends import FallbackCache
 
 
-def test_fallback_cache_get(mocker, settings):
-    """Test that get() on the fallback cache works correctly"""
-    mock_cache_1 = mocker.Mock(spec=BaseCache)
-    mock_cache_1.get.return_value = 12345
-    mock_cache_2 = mocker.Mock(spec=BaseCache)
-    mock_cache_2.get.return_value = 67890
+@dataclass
+class MockCaches:
+    caches: list[BaseCache]
+    cache_names: list[str]
+    caches_by_name: dict[str, BaseCache]
+    cache_results: list[str]
 
-    mocker.patch.dict(
-        "main.cache.backends.caches", {"dummy1": mock_cache_1, "dummy2": mock_cache_2}
-    )
 
-    cache = FallbackCache(["dummy1", "dummy2"], {})
+@pytest.fixture(autouse=True)
+def mock_caches(mocker):
+    """Define mock caches"""
+    cache_names = []
+    caches = []
+    caches_by_name = {}
+    cache_results = []
+
+    for idx in range(3):
+        name = f"dummy-{idx}"
+        result = f"result-{idx}"
+        mock_cache = mocker.Mock(spec=BaseCache)
+        mock_cache.get.return_value = result
 
-    assert cache.get("key", default="default", version=1) == 12345
+        cache_names.append(name)
+        caches.append(mock_cache)
+        caches_by_name[name] = mock_cache
+        cache_results.append(result)
 
-    mock_cache_1.get.return_value = None
+    mocker.patch.dict("main.cache.backends.caches", caches_by_name)
 
-    assert cache.get("key", default="default", version=1) == 67890
+    return MockCaches(caches, cache_names, caches_by_name, cache_results)
 
-    mock_cache_2.get.return_value = None
 
-    assert cache.get("key", default="default", version=1) is None
+@pytest.mark.parametrize("cache_hit_idx", range(4))
+def test_fallback_cache_get(mock_caches: MockCaches, settings, cache_hit_idx):
+    """Test that get() on the fallback cache works correctly"""
 
+    cache = FallbackCache(mock_caches.cache_names, {})
 
-def test_fallback_cache_set(mocker, settings):
-    """Test that set() on the fallback cache works correctly"""
-    mock_cache_1 = mocker.Mock(spec=BaseCache)
-    mock_cache_2 = mocker.Mock(spec=BaseCache)
+    cold_caches = mock_caches.caches[:cache_hit_idx]
 
-    mocker.patch.dict(
-        "main.cache.backends.caches", {"dummy1": mock_cache_1, "dummy2": mock_cache_2}
+    for mock_cache in cold_caches:
+        mock_cache.get.return_value = None
+
+    caches_exhausted = cache_hit_idx >= len(mock_caches.caches)
+    expected_value = (
+        "default" if caches_exhausted else mock_caches.cache_results[cache_hit_idx]
     )
 
-    cache = FallbackCache(["dummy1", "dummy2"], {})
+    assert cache.get("key", default="default", version=1) == expected_value
+
+    if not caches_exhausted:
+        for mock_cache in cold_caches:
+            mock_cache.set.assert_called_once_with(
+                "key", expected_value, timeout=cache.get_backend_timeout(), version=1
+            )
+
+
+@pytest.mark.parametrize("cache_timeout", [DEFAULT_TIMEOUT, None, 0, 1000])
+@pytest.mark.parametrize(
+    "kwargs",
+    [
+        {},
+        {"timeout": 600},
+        {"version": 1},
+        {"timeout": 600, "version": 1},
+    ],
+)
+def test_fallback_cache_set(mock_caches, settings, cache_timeout, kwargs):
+    """Test that set() on the fallback cache works correctly"""
+    cache = FallbackCache(mock_caches.cache_names, {"TIMEOUT": cache_timeout})
+    cache.set("key", "value", **kwargs)
 
-    cache.set("key", "value", timeout=600, version=1)
+    expected_timeout = kwargs.get("timeout", cache_timeout)
+    expected_version = kwargs.get("version", None)
 
-    mock_cache_1.set.assert_called_once_with("key", "value", timeout=600, version=1)
-    mock_cache_2.set.assert_called_once_with("key", "value", timeout=600, version=1)
+    for mock_cache in mock_caches.caches:
+        mock_cache.set.assert_called_once_with(
+            "key", "value", timeout=expected_timeout, version=expected_version
+        )

main/settings.py

@@ -537,9 +537,21 @@
     "imagekit": {
         "BACKEND": "main.cache.backends.FallbackCache",
         "LOCATION": [
+            "imagekit_redis",
             "imagekit_db",
         ],
     },
+    # This uses FallbackCache but it's basically a proxy to the redis cache
+    # so that we can reuse the client and not create another pile of connections.
+    # The main purpose of this is to set TIMEOUT without specifying it on
+    # the global redis cache.
+    "imagekit_redis": {
+        "BACKEND": "main.cache.backends.FallbackCache",
+        "LOCATION": [
+            "redis",
+        ],
+        "TIMEOUT": 60 * 60,
+    },
     "imagekit_db": {
         "BACKEND": "django.core.cache.backends.db.DatabaseCache",
         "LOCATION": "imagekit_cache",