First cut at Request cache

common/djangoapps/request_cache/__init__.py

@@ -8,6 +8,8 @@ is installed in order to clear the cache after each request.
 import logging
 from urlparse import urlparse
 
+from django.core.cache import caches
+from django.core.cache.backends.base import BaseCache
 from django.conf import settings
 from django.test.client import RequestFactory
 
@@ -69,3 +71,147 @@ def get_request_or_stub():
 
     else:
         return request
+
+
+class RequestPlusRemoteCache(BaseCache):
+    """
+    This Django cache backend implements two layers of caching.
+
+    Layer 1 is a threadlocal dictionary that is tied to the life of a given
+    request.
+
+    Some baseline rules:
+
+    1. Treat it as a global namespace, like any other cache. The per-request
+       local cache is only going to live for the lifetime of one request, but
+       the backing cache is going to something like Memcached, where key
+       collision is possible.
+
+    2. Timeouts are ignored for the purposes of the in-memory request cache, but
+       do apply to the backing remote cache. One consequence of this is that
+       sending an explicit timeout of 0 in `set` or `add` will cause that item
+       to only be cached across the duration of the request and never make it
+       out to the backing remote cache.
+    """
+    def __init__(self, name, params):
+        try:
+            super(RequestPlusRemoteCache, self).__init__(params)
+            self._remote_cache = caches[params['REMOTE_CACHE_NAME']]
+        except Exception:
+            log.exception(
+                "DjangoRequestCache %s could not load backing remote cache.",
+                name
+            )
+            raise
+
+        # This is a threadlocal that will get wiped out for each request.
+        self._local_dict = get_cache("DjangoRequestCache")
+
+    def add(self, key, value, timeout=0, version=None):
+        """
+        Set a value in the cache if the key does not already exist. If
+        timeout is given, that timeout will be used for the key; otherwise
+        the timeout will default to 0, and the (key, value) will only be stored
+        in the local in-memory request cache, not the backing remote cache.
+
+        Returns True if the value was stored, False otherwise.
+        """
+        local_key = self.make_key(key, version)
+        if local_key in self._local_dict:
+            return False
+
+        self._local_dict[local_key] = value
+        if timeout != 0:
+            self._remote_cache.add(key, value, timeout=timeout, version=version)
+
+        return True
+
+    def get(self, key, default=None, version=None):
+        """
+        Fetch a given key from the cache. If the key does not exist, return
+        default, which itself defaults to None.
+        """
+        # Simple case: It's already in our local memory...
+        local_key = self.make_key(key, version)
+        if local_key in self._local_dict:
+            return self._local_dict[local_key]
+
+        # Now try looking it up in our backing cache...
+        external_value = self._remote_cache.get(key, default=default, version=None)
+
+        # This might be None, but we store it anyway to prevent repeated requests
+        # to the same non-existent key during the course of the request.
+        self._local_dict[local_key] = external_value
+
+        return external_value
+
+    def set(self, key, value, timeout=0, version=None):
+        """
+        Set a value in the cache. If timeout is given, that timeout will be used
+        for the key when storing in the remote cache; otherwise the timeout will
+        default to 0, and the (key, value) will only be stored in the local
+        in-memory request cache.
+
+        For example::
+
+            # This will only be stored in the local request cache, and should
+            # be used for items where there are potentially many, many keys.
+            dj_req_cache.set('has_access:user1243:block3048', True, 0)
+
+            # This value will be stored in both the local request cache and the
+        """
+        local_key = self.make_key(key, version)
+        self._local_dict[local_key] = value
+        if timeout != 0:
+            self._remote_cache.set(key, value, timeout=timeout, version=version)
+
+    def delete(self, key, version=None):
+        """
+        Delete a key from the cache, failing silently.
+
+        Note that this *will* flow through to the backing remote cache.
+        """
+        local_key = self.make_key(key, version)
+        if local_key in self._local_:
+            del self._local_dict[local_key]
+        self._remote_cache.delete(key, version=version)
+
+    def get_many(self, keys, version=None):
+        mapping = {}
+
+        # First get all the keys that exist locally.
+        for key in keys:
+            local_key = self.make_key(key)
+            if local_key in self._local_dict:
+                mapping[key] = self._local_dict[local_key]
+
+        # Now check the external cache for everything that we didn't find
+        remaining_keys = set(keys) - set(mapping)
+        external_mapping = self._remote_cache.get_many(remaining_keys, version=version)
+
+        # Update both the mapping that we're returning as well as our local cache
+        mapping.update(external_mapping)
+        self._local_dict.update({
+            self.make_key(key): value for key, value in external_mapping.items()
+        })
+
+        return mapping
+
+    def set_many(self, data, timeout=0, version=None):
+        self._local_dict.update({
+            self.make_key(key): value for key, value in data.items()
+        })
+        self._remote_cache.set_many(data, timeout=timeout, version=version)
+
+    def delete_many(self, keys, version=None):
+        for key in keys:
+            del self._local_dict[self.make_key(key)]
+        self._remote_cache.delete_many(keys)
+
+    def clear(self):
+        self._local_dict.clear()
+        self._remote_cache.clear()
+
+    def close(self, **kwargs):
+        self._local_dict.clear()
+        self._remote_cache.close()

common/djangoapps/request_cache/tests.py

@@ -18,3 +18,9 @@ class TestRequestCache(TestCase):
         stub = get_request_or_stub()
         expected_url = "http://{site_name}/foobar".format(site_name=settings.SITE_NAME)
         self.assertEqual(stub.build_absolute_uri("foobar"), expected_url)
+
+
+class TestDjangoRequestCache(TestCase):
+
+    pass
+

common/lib/xmodule/xmodule/discussion_module.py

@@ -57,6 +57,7 @@ def has_permission(user, permission, course_id):
 
 
 @XBlock.wants('user')
+@XBlock.wants('cache')
 class DiscussionModule(DiscussionFields, XModule):
     """
     XModule for discussion forums.
@@ -77,11 +78,12 @@ class DiscussionModule(DiscussionFields, XModule):
         user_service = self.runtime.service(self, 'user')
         if user_service:
             user = user_service._django_user  # pylint: disable=protected-access
+
         if user:
-            course_key = course.id
-            can_create_comment = has_permission(user, "create_comment", course_key)
-            can_create_subcomment = has_permission(user, "create_sub_comment", course_key)
-            can_create_thread = has_permission(user, "create_thread", course_key)
+            course_key = self.course_id
+            can_create_comment = self.has_permission(user, "create_comment", course_key)
+            can_create_subcomment = self.has_permission(user, "create_sub_comment", course_key)
+            can_create_thread = self.has_permission(user, "create_thread", course_key)
         else:
             can_create_comment = False
             can_create_subcomment = False
@@ -99,12 +101,21 @@ class DiscussionModule(DiscussionFields, XModule):
             template = 'discussion/_discussion_module.html'
         return self.system.render_template(template, context)
 
-    def get_course(self):
+    def has_permission(self, user, permission, course_id):
         """
-        Return CourseDescriptor by course id.
+        Copied from django_comment_client/permissions.py because I can't import
+        that file from here. It causes the xmodule_assets command to fail.
         """
-        course = self.runtime.modulestore.get_course(self.course_id)
-        return course
+        cache = self.runtime.service(self, 'cache').get_cache("discussion.permissions")
+        cache_key = (user, course_id, permission)
+        cached_answer = cache.get(cache_key)
+        if cached_answer is not None:
+            return cached_answer
+
+        has_perm = any(role.has_permission(permission) for role in user.roles.filter(course_id=course_id))
+        cache.set(cache_key, has_perm, 0)
+
+        return has_perm
 
 
 class DiscussionDescriptor(DiscussionFields, MetadataOnlyEditingDescriptor, RawDescriptor):

lms/djangoapps/lms_xblock/runtime.py

@@ -6,6 +6,9 @@ import re
 from django.conf import settings
 from django.core.urlresolvers import reverse
 
+from django.core.cache import caches
+import request_cache
+
 from badges.service import BadgingService
 from badges.utils import badges_enabled
 from openedx.core.djangoapps.user_api.course_tag import api as user_course_tag_api
@@ -194,6 +197,96 @@ class UserTagsService(object):
         )
 
 
+class CacheService(object):
+    """
+
+    """
+    def __init__(self, req_cache_root, django_cache=None):
+        """
+        Create a CacheService that can generate named Caches for XBlocks.
+
+        `req_cache_root` should be a `dict`-like object, and will be where we
+        store the in-memory cached version of various objects during a single
+        request. Each named cache that is returned by `get_cache()` will create
+        a new entry here. So for example, if you did the following from inside
+        an XBlock:
+
+        cache_service = self.runtime.service(self, 'cache')
+        perm_cache = cache_service.get_cache("discussion.permissions")
+        tmpl_cache = cache_service.get_cache("discussion.templates")
+
+        Then `req_cache_root` would look like:
+
+          {
+              "discussion.permissions": {},
+              "discussion.templates": {}
+          }
+
+        `django_cache` is the explicit Django low level cache object we want to
+        use, in case there's a specific named cache that is preferred. By
+        default, it'll just use django.core.cache.cache.
+        """
+
+        # Create an entry in the request_cache where we will put all
+        # CacheService generated cache data. We'll further namespace with our
+        # own get_cache() method. So:
+        #
+        # request_cache = {
+        #
+        # }
+        self._local_cache_root = local_cache_root
+        self._django_cache = django_cache or caches['request']
+
+    def get_cache(self, name):
+        """
+        Return a named Cache object that get/set can be called on.
+
+        This is to do simple namespacing, and is not intended to guard against
+        malicious behavior. The `name` parameter should include your XBlock's
+        tag as a prefix parameter.
+        """
+        if name not in self._request_cache:
+            self._request_cache[name] = {}
+
+        return Cache(name, self._request_cache, self._django_cache)
+
+
+class Cache(object):
+    """
+    Low level Cache object for use by an XBlock.
+
+    To get an instance of this from within an XBlock, you would do something
+    like::
+
+        cache = self.runtime.service(self, 'cache').get_cache("myxblock")
+        cached_value = cache.get(key)
+        cache.set(key, value, timeout)
+
+    """
+
+    def __init__(self, name, cache_dict, django_cache):
+        self._cache_dict = cache_dict
+        self._django_cache = django_cache
+
+    def get(self, key):
+        pass
+
+    def get_many(self, keys):
+        pass
+
+    def set(self, key, value, timeout, version=1):
+        """
+        Set key -> value mapping in cache.
+
+        `key` should be hashable
+
+        """
+        self._cache_dict[key] = value
+
+    def set_many(self, kv_dict, timeout):
+        pass
+
+
 class LmsModuleSystem(ModuleSystem):  # pylint: disable=abstract-method
     """
     ModuleSystem specialized to the LMS
@@ -215,6 +308,9 @@ class LmsModuleSystem(ModuleSystem):  # pylint: disable=abstract-method
         services['user_tags'] = UserTagsService(self)
         if badges_enabled():
             services['badging'] = BadgingService(course_id=kwargs.get('course_id'), modulestore=store)
+        services['cache'] = CacheService(request_cache.get_cache("xblock_cache_service"))
+
+
         self.request_token = kwargs.pop('request_token', None)
         super(LmsModuleSystem, self).__init__(**kwargs)
 

lms/envs/aws.py

@@ -201,6 +201,13 @@ if 'loc_cache' not in CACHES:
         'LOCATION': 'edx_location_mem_cache',
     }
 
+# Create a request cache if one is not already specified.
+if 'request' not in CACHES:
+    CACHES['request'] = {
+        'BACKEND': 'request_cache.RequestPlusRemoteCache',
+        'REMOTE_CACHE_NAME': 'default',
+    }
+
 # Email overrides
 DEFAULT_FROM_EMAIL = ENV_TOKENS.get('DEFAULT_FROM_EMAIL', DEFAULT_FROM_EMAIL)
 DEFAULT_FEEDBACK_EMAIL = ENV_TOKENS.get('DEFAULT_FEEDBACK_EMAIL', DEFAULT_FEEDBACK_EMAIL)

lms/envs/test.py

@@ -233,6 +233,12 @@ CACHES = {
     'course_structure_cache': {
         'BACKEND': 'django.core.cache.backends.dummy.DummyCache',
     },
+
+    ##### ADD REQUEST CACHE HERE
+    'request': {
+        'BACKEND': 'request_cache.RequestPlusRemoteCache',
+        'REMOTE_CACHE_NAME': 'default',
+    }
 }
 
 # Dummy secret key for dev