perf: ad caching to many functions

futurex_openedx_extensions/__init__.py

@@ -1,3 +1,3 @@
 """One-line description for README and other doc files."""
 
-__version__ = '0.3.5'
+__version__ = '0.3.6'

futurex_openedx_extensions/dashboard/settings/common_production.py

@@ -1,8 +1,13 @@
 """Common Settings"""
 
 
-def plugin_settings(settings):  # pylint: disable=unused-argument
+def plugin_settings(settings):
     """
     plugin settings
     """
-    # Nothing to do here yet
+    # Cache timeout for tenants info
+    settings.FX_CACHE_TIMEOUT_TENANTS_INFO = getattr(
+        settings,
+        "FX_CACHE_TIMEOUT_TENANTS_INFO",
+        60 * 60 * 2,  # 2 hours
+    )

futurex_openedx_extensions/helpers/caching.py

@@ -0,0 +1,47 @@
+"""Helper functions for caching"""
+import functools
+import logging
+
+from django.core.cache import cache
+
+log = logging.getLogger(__name__)
+
+
+def cache_dict(timeout, key_generator_or_name):
+    """Cache the dictionary result returned by the function"""
+    def decorator(func):
+        """Decorator definition"""
+        @functools.wraps(func)
+        def wrapped(*args, **kwargs):
+            """Wrapped function"""
+            cache_key = None
+            try:
+                if not isinstance(timeout, int) or timeout <= 0:
+                    raise ValueError(
+                        "unexpected timout value. Should be an integer greater than 0"
+                    )
+                if not callable(key_generator_or_name) and not isinstance(key_generator_or_name, str):
+                    raise TypeError("key_generator_or_name must be a callable or a string")
+
+                cache_key = key_generator_or_name(
+                    *args, **kwargs
+                ) if callable(key_generator_or_name) else key_generator_or_name
+
+            except Exception as exc:  # pylint: disable=broad-except
+                log.exception("cache_dict: error generating cache key: %s", exc)
+
+            result = cache.get(cache_key) if cache_key else None
+            if result is None:
+                result = func(*args, **kwargs)
+                if cache_key and result and isinstance(result, dict):
+                    cache.set(cache_key, result, timeout)
+                elif cache_key and result:
+                    # log: cache_dict: expecting dictionary result from <<function name>> but got <<result type>>
+                    log.error(
+                        "cache_dict: expecting dictionary result from %s but got %s",
+                        func.__name__, type(result)
+                    )
+            return result
+
+        return wrapped
+    return decorator

futurex_openedx_extensions/helpers/tenants.py

@@ -4,12 +4,14 @@
 from typing import Any, Dict, List
 
 from common.djangoapps.student.models import CourseAccessRole, CourseEnrollment
+from django.conf import settings
 from django.contrib.auth import get_user_model
 from django.db.models import Exists, OuterRef
 from django.db.models.query import Q, QuerySet
 from eox_tenant.models import Route, TenantConfig
 from rest_framework.request import Request
 
+from futurex_openedx_extensions.helpers.caching import cache_dict
 from futurex_openedx_extensions.helpers.converters import error_details_to_dictionary, ids_string_to_list
 from futurex_openedx_extensions.helpers.querysets import get_has_site_login_queryset
 
@@ -50,9 +52,9 @@ def get_all_tenants() -> QuerySet:
     return TenantConfig.objects.exclude(id__in=get_excluded_tenant_ids())
 
 
+@cache_dict(timeout=settings.FX_CACHE_TIMEOUT_TENANTS_INFO, key_generator_or_name='all_tenants_info')
 def get_all_tenants_info() -> Dict[str, Any]:
     """
-    TODO: Cache the result of this function
     Get all tenants in the system that are exposed in the route table, and with a valid config
 
     Note: a tenant is a TenantConfig object
@@ -92,9 +94,9 @@ def get_tenant_site(tenant_id: int) -> str:
     return get_all_tenants_info()['sites'].get(tenant_id)
 
 
+@cache_dict(timeout=settings.FX_CACHE_TIMEOUT_TENANTS_INFO, key_generator_or_name='all_course_org_filter_list')
 def get_all_course_org_filter_list() -> Dict[int, List[str]]:
     """
-    TODO: Cache the result of this function
     Get all course org filters for all tenants.
 
     :return: Dictionary of tenant IDs and their course org filters

test_settings.py

@@ -72,3 +72,13 @@ def root(*args):
 DEFAULT_AUTO_FIELD = 'django.db.models.AutoField'
 
 USERNAME_PATTERN = r'(?P<username>[\w.@+-]+)'
+
+# Ensure that the cache is volatile in tests
+CACHES = {
+    'default': {
+        'BACKEND': 'django.core.cache.backends.dummy.DummyCache',
+    }
+}
+
+# Non-default dashboard settings
+FX_CACHE_TIMEOUT_TENANTS_INFO = 60 * 60 * 3  # 3 hours

tests/test_helpers/test_apps.py

@@ -1,7 +1,45 @@
 """Tests for the apps module of the helpers app"""
+import copy
+
+import pytest
+
+from futurex_openedx_extensions.dashboard.settings import common_production
 from futurex_openedx_extensions.helpers.apps import HelpersConfig
 
+dashboard_default_settings = [
+    ('FX_CACHE_TIMEOUT_TENANTS_INFO', 60 * 60 * 2),  # 2 hours
+]
+
 
 def test_app_name():
     """Test that the app name is correct"""
     assert HelpersConfig.name == 'futurex_openedx_extensions.helpers'
+
+
+def test_common_production_plugin_settings():
+    """Verify settings contain the method plugin_settings"""
+    assert hasattr(common_production, 'plugin_settings'), 'settings is missing the method plugin_settings!'
+
+
+@pytest.mark.parametrize('setting_key, default_value', dashboard_default_settings)
+def test_common_production_plugin_settings_new_attributes(settings, setting_key, default_value):
+    """Verify that the plugin's settings contain the new settings"""
+    settings = copy.deepcopy(settings)
+    delattr(settings, setting_key)
+    assert not hasattr(settings, setting_key), 'whaaaat?!!!'
+
+    common_production.plugin_settings(settings)
+    assert hasattr(settings, setting_key), f'Missing settings ({setting_key})!'
+    assert getattr(settings, setting_key) == default_value, f'Unexpected settings value ({setting_key})!'
+
+
+@pytest.mark.parametrize('setting_key, default_value', dashboard_default_settings)
+def test_common_production_plugin_settings_explicit(settings, setting_key, default_value):
+    """Verify that the plugin's settings read from env"""
+    settings = copy.deepcopy(settings)
+
+    new_value = getattr(settings, setting_key)
+    assert new_value != default_value, 'Bad test data, default value is the same as the new value!'
+    common_production.plugin_settings(settings)
+    assert hasattr(settings, setting_key), f'Missing settings ({setting_key})!'
+    assert getattr(settings, setting_key) == new_value, f'settings ({setting_key}) did not read from env correctly!'

tests/test_helpers/test_caching.py

@@ -0,0 +1,126 @@
+"""Tests for caching helper functions."""
+from unittest.mock import patch
+
+import pytest
+from django.core.cache import cache
+
+from futurex_openedx_extensions.helpers.caching import cache_dict
+
+
+@pytest.fixture
+def mock_cache():
+    with patch.object(cache, 'get') as mock_get:
+        with patch.object(cache, 'set') as mock_set:
+            yield mock_get, mock_set
+
+
+@cache_dict(timeout=77, key_generator_or_name='test_key')
+def dummy_cached_func():
+    return {'key': 'value'}
+
+
+def test_cache_hit(mock_cache):  # pylint: disable=redefined-outer-name
+    """Verify that the cache is hit and the result is returned."""
+    mock_get, mock_set = mock_cache
+    mock_get.return_value = {'key': 'value'}
+
+    result = dummy_cached_func()
+    assert result == {'key': 'value'}
+    mock_get.assert_called_once_with('test_key')
+    mock_set.assert_not_called()
+
+
+def test_cache_miss(mock_cache):  # pylint: disable=redefined-outer-name
+    """Verify that the cache is missed and the result is stored."""
+    mock_get, mock_set = mock_cache
+    mock_get.return_value = None
+
+    result = dummy_cached_func()
+    assert result == {'key': 'value'}
+    mock_get.assert_called_once_with('test_key')
+    mock_set.assert_called_once_with('test_key', {'key': 'value'}, 77)
+
+
+def test_cache_key_callable(mock_cache):  # pylint: disable=redefined-outer-name
+    """Verify that the cache key is generated by a callable."""
+    mock_get, mock_set = mock_cache
+    mock_get.return_value = None
+
+    def key_gen(*args, **kwargs):
+        return f"test_key_{args[0]}_{args[1]}"
+
+    @cache_dict(timeout=88, key_generator_or_name=key_gen)
+    def dummy_func(arg1, arg2):
+        return {arg2: arg1}
+
+    result = dummy_func(1, 'str')
+    assert result == {'str': 1}
+    mock_get.assert_called_once_with('test_key_1_str')
+    mock_set.assert_called_once_with('test_key_1_str', {'str': 1}, 88)
+
+
+def test_cache_key_generation_error(mock_cache, caplog):  # pylint: disable=redefined-outer-name
+    """Verify that an error generating the cache key is logged."""
+    mock_get, mock_set = mock_cache
+
+    def key_gen(*args, **kwargs):
+        raise ValueError("Error generating key")
+
+    @cache_dict(timeout=60, key_generator_or_name=key_gen)
+    def dummy_func():
+        return {'key': 'value'}
+
+    result = dummy_func()
+    assert result == {'key': 'value'}
+    mock_get.assert_not_called()
+    mock_set.assert_not_called()
+    assert "cache_dict: error generating cache key" in caplog.text
+
+
+def test_cache_incorrect_result_type(mock_cache, caplog):  # pylint: disable=redefined-outer-name
+    """Verify that an error is logged when the result is not a dictionary."""
+    mock_get, mock_set = mock_cache
+    mock_get.return_value = None
+
+    @cache_dict(timeout=77, key_generator_or_name='test_key')
+    def dummy_func():
+        return ['not', 'a', 'dict']
+
+    result = dummy_func()
+    assert result == ['not', 'a', 'dict']
+    mock_get.assert_called_once_with('test_key')
+    mock_set.assert_not_called()
+    assert "cache_dict: expecting dictionary result from dummy_func but got <class 'list'>" in caplog.text
+
+
+def test_cache_key_not_callable_or_string(mock_cache, caplog):  # pylint: disable=redefined-outer-name
+    """Verify that an error is logged when the key generator is not callable or a string."""
+    mock_get, mock_set = mock_cache
+
+    @cache_dict(timeout=60, key_generator_or_name=123)
+    def dummy_func():
+        return {'key': 'value'}
+
+    result = dummy_func()
+    assert result == {'key': 'value'}
+    mock_get.assert_not_called()
+    mock_set.assert_not_called()
+    assert "cache_dict: error generating cache key: key_generator_or_name must be a callable or a string" in caplog.text
+
+
+@pytest.mark.parametrize('timeout', [-1, 0, None, 77.5])
+def test_bad_timeout(mock_cache, caplog, timeout):  # pylint: disable=redefined-outer-name
+    """Verify that an error is logged when the timeout is not a positive integer."""
+    mock_get, mock_set = mock_cache
+    mock_get.return_value = None
+
+    @cache_dict(timeout=timeout, key_generator_or_name='test_key')
+    def dummy_func():
+        return {'key': 'value'}
+
+    result = dummy_func()
+    assert result == {'key': 'value'}
+    mock_get.assert_not_called()
+    mock_set.assert_not_called()
+    assert "cache_dict: error generating cache key: unexpected timout value. Should be an integer greater than 0" in \
+           caplog.text

tests/test_helpers/test_tenants.py

@@ -3,6 +3,8 @@
 import pytest
 from common.djangoapps.student.models import CourseEnrollment, UserSignupSource
 from django.contrib.auth import get_user_model
+from django.core.cache import cache
+from django.test import override_settings
 from eox_tenant.models import TenantConfig
 
 from futurex_openedx_extensions.helpers import tenants
@@ -166,6 +168,15 @@ def test_get_all_course_org_filter_list(base_data):  # pylint: disable=unused-ar
     }
 
 
+@override_settings(CACHES={'default': {'BACKEND': 'django.core.cache.backends.locmem.LocMemCache'}})
+@pytest.mark.django_db
+def test_get_all_course_org_filter_list_is_being_cached():
+    """Verify that get_all_course_org_filter_list is being cached."""
+    assert cache.get('all_course_org_filter_list') is None
+    result = tenants.get_all_course_org_filter_list()
+    assert cache.get('all_course_org_filter_list') == result
+
+
 @pytest.mark.django_db
 @pytest.mark.parametrize("tenant_ids, expected", [
     ([1, 2, 3, 7], {
@@ -234,6 +245,15 @@ def test_get_all_tenants_info(base_data):  # pylint: disable=unused-argument
     }
 
 
+@override_settings(CACHES={'default': {'BACKEND': 'django.core.cache.backends.locmem.LocMemCache'}})
+@pytest.mark.django_db
+def test_get_all_tenants_info_is_being_cached():
+    """Verify that get_all_tenants_info is being cached."""
+    assert cache.get('all_tenants_info') is None
+    result = tenants.get_all_tenants_info()
+    assert cache.get('all_tenants_info') == result
+
+
 @pytest.mark.django_db
 @pytest.mark.parametrize("tenant_id, expected", [
     (1, 's1.sample.com'),