fix paradigm to avoid incorrect usages

It's almost impossible to create a decorator that works on both methods
and functions, but more importantly the original approach was sharing a
cache across instances of the policy. It needed to be local to the
policy instance, but shared across requests. The new example
demonstrates that.

The cache is also much more flexible in its usage patterns now.

3 files changed, 136 insertions, 90 deletions

diff --git a/docs/api/request.rst b/docs/api/request.rst
index 59d85ac2a..357e49b76 100644
--- a/docs/api/request.rst
+++ b/docs/api/request.rst
@@ -365,3 +365,4 @@
 
 .. autoclass:: RequestLocalCache
     :members:
+    :special-members:
diff --git a/src/pyramid/request.py b/src/pyramid/request.py
index d79aad9bf..0d3cf481b 100644
--- a/src/pyramid/request.py
+++ b/src/pyramid/request.py
@@ -339,41 +339,41 @@ class RequestLocalCache:
     """
     A store that caches values during for the lifecycle of a request.
 
-    Instantiate a cache object and use it to decorate methods or functions
-    that accept a request parameter. Any other arguments are not used as
-    cache keys so make sure they are constant across calls per-request.
+    Wrapping Functions
 
-    Wrapping methods:
+    Instantiate and use it to decorate functions that accept a request
+    parameter. The result is cached and returned in subsequent invocations
+    of the function.
 
     .. code-block:: python
 
-        class SecurityPolicy:
-            identity_cache = RequestLocalCache()
+        @RequestLocalCache()
+        def get_user(request):
+            result = ...  # do some expensive computations
+            return result
 
-            @identity_cache
-            def authenticated_identity(self, request):
-                result = ...  # do some expensive computations
-                return result
+    Wrapping Methods
 
-    Wrapping functions:
+    A method can be wrapped but it needs to be bound to an instance such that
+    it only accepts one argument - the request.
 
     .. code-block:: python
 
-        user_cache = RequestLocalCache()
+        class SecurityPolicy:
+            def __init__(self):
+                self.identity_cache = RequestLocalCache(self.load_identity)
 
-        @user_cache
-        def get_user(request):
-            result = ...  # do some expensive computations
-            return result
+            def load_identity(self, request):
+                result = ...  # do some expensive computations
+                return result
 
-    It's also possible to inspect and influence the cache during runtime using
-    :meth:`.get`, :meth:`.set` and :meth:`.clear`. Using these methods, the
-    cache can be used directly as well, without using it as a decorator.
+            def authenticated_identity(self, request):
+                return self.identity_cache.get_or_create(request)
 
-    The cache will release resources aggressively by utilizing
-    :meth:`pyramid.request.Request.add_finished_callback`, but it will also
-    maintain a weakref to the request and cleanup when it is garbage collected
-    if the callbacks are not invoked for some reason.
+    The cache maintains a weakref to each request and will release the cached
+    values when the request is garbage-collected. However, in most scenarios,
+    it will release resources earlier via
+    :meth:`pyramid.request.Request.add_finished_callback`.
 
     .. versionadded:: 2.0
 
@@ -381,56 +381,62 @@ class RequestLocalCache:
 
     NO_VALUE = Sentinel('NO_VALUE')
 
-    def __init__(self):
-        self.store = weakref.WeakKeyDictionary()
+    def __init__(self, creator=None):
+        self._store = weakref.WeakKeyDictionary()
+        self._creator = creator
 
-    def auto_adapt(decorator):
-        class FunctionOrMethodAdapter:
-            def __init__(self, inst, func):
-                self.inst = inst
-                self.__wrapped__ = func
+    def __call__(self, fn):
+        """
+        Decorate and return a new function that utilizes the cache.
 
-            def __call__(self, *args, **kwargs):
-                return decorator(self.inst, self.__wrapped__)(*args, **kwargs)
+        The cache is attached as an attribute to the decorated function
+        such that it may be manipulated directly. For example:
 
-            def __get__(self, instance, owner):
-                return decorator(
-                    self.inst, self.__wrapped__.__get__(instance, owner)
-                )
+        .. code-block:: python
 
-        def adapt(inst, fn):
-            return FunctionOrMethodAdapter(inst, fn)
+            @RequestLocalCache()
+            def do_something_expensive(request):
+                return ...
 
-        return adapt
+            value = do_something_expensive(request)
+            do_something_expensive.cache.clear(request)
 
-    @auto_adapt
-    def __call__(self, fn):
-        """
-        Decorate a method or function.
+        The ``fn`` is also bound as the creator on the cache such that
+        invocations of :meth:`.get_or_create` will use it.
 
         """
 
         @functools.wraps(fn)
-        def wrapper(request, *args, **kwargs):
-            result = self.get(request)
-            if result is self.NO_VALUE:
-                result = fn(request, *args, **kwargs)
-                self.set(request, result)
-                request.add_finished_callback(self.clear)
-            return result
+        def wrapper(request):
+            return wrapper.cache.get_or_create(request, fn)
 
+        wrapper.cache = self
+        self._creator = fn
         return wrapper
 
-    del auto_adapt
-
-    def clear(self, request):
+    def get_or_create(self, request, creator=None):
         """
-        Delete the value from the cache.
+        Return the cached value.
 
-        The cached value is returned or :attr:`.NO_VALUE`.
+        If no value is cached then execute the creator, cache the result,
+        and return it.
+
+        The creator may be passed in as an argument or bound to the cache
+        using the ``__call__`` or constructor arguments.
 
         """
-        return self.store.pop(request, self.NO_VALUE)
+        result = self._store.get(request, self.NO_VALUE)
+        if result is self.NO_VALUE:
+            if creator is None:
+                creator = self._creator
+                if creator is None:
+                    raise ValueError(
+                        'no creator function has been registered with the '
+                        'cache or supplied to "get_or_create"'
+                    )
+            result = creator(request)
+            self.set(request, result)
+        return result
 
     def get(self, request, default=NO_VALUE):
         """
@@ -439,11 +445,32 @@ class RequestLocalCache:
         The cached value is returned or ``default``.
 
         """
-        return self.store.get(request, default)
+        return self._store.get(request, default)
 
     def set(self, request, value):
         """
         Update the cache with a new value.
 
         """
-        self.store[request] = value
+        already_set = request in self._store
+        self._store[request] = value
+
+        # avoid registering the callback more than once
+        if not already_set:
+            request.add_finished_callback(self._store.pop)
+
+    def clear(self, request):
+        """
+        Delete the value from the cache.
+
+        The cached value is returned or :attr:`.NO_VALUE`.
+
+        """
+        old_value = self.NO_VALUE
+        if request in self._store:
+            old_value = self._store[request]
+
+            # keep a value in the store so that we don't register another
+            # finished callback when set is invoked
+            self._store[request] = self.NO_VALUE
+        return old_value
diff --git a/tests/test_request.py b/tests/test_request.py
index 3cff4bb53..a36bd238c 100644
--- a/tests/test_request.py
+++ b/tests/test_request.py
@@ -595,16 +595,15 @@ class Test_subclassing_Request(unittest.TestCase):
 
 
 class TestRequestLocalCache(unittest.TestCase):
-    def _makeOne(self):
+    def _makeOne(self, *args, **kwargs):
         from pyramid.request import RequestLocalCache
 
-        return RequestLocalCache()
+        return RequestLocalCache(*args, **kwargs)
 
     def test_it_works_with_functions(self):
-        cache = self._makeOne()
         a = [0]
 
-        @cache
+        @self._makeOne()
         def foo(request):
             a[0] += 1
             return a[0]
@@ -618,31 +617,10 @@ class TestRequestLocalCache(unittest.TestCase):
         self.assertEqual(len(req1.finished_callbacks), 1)
         self.assertEqual(len(req2.finished_callbacks), 1)
 
-    def test_it_works_with_methods(self):
-        cache = self._makeOne()
-        a = [0]
-
-        class DummyPolicy:
-            @cache
-            def foo(self, request):
-                a[0] += 1
-                return a[0]
-
-        policy = DummyPolicy()
-        req1 = DummyRequest()
-        req2 = DummyRequest()
-        self.assertEqual(policy.foo(req1), 1)
-        self.assertEqual(policy.foo(req2), 2)
-        self.assertEqual(policy.foo(req1), 1)
-        self.assertEqual(policy.foo(req2), 2)
-        self.assertEqual(len(req1.finished_callbacks), 1)
-        self.assertEqual(len(req2.finished_callbacks), 1)
-
     def test_clear_works(self):
-        cache = self._makeOne()
         a = [0]
 
-        @cache
+        @self._makeOne()
         def foo(request):
             a[0] += 1
             return a[0]
@@ -650,15 +628,14 @@ class TestRequestLocalCache(unittest.TestCase):
         req = DummyRequest()
         self.assertEqual(foo(req), 1)
         self.assertEqual(len(req.finished_callbacks), 1)
-        cache.clear(req)
+        foo.cache.clear(req)
         self.assertEqual(foo(req), 2)
-        self.assertEqual(len(req.finished_callbacks), 2)
+        self.assertEqual(len(req.finished_callbacks), 1)
 
     def test_set_overrides_current_value(self):
-        cache = self._makeOne()
         a = [0]
 
-        @cache
+        @self._makeOne()
         def foo(request):
             a[0] += 1
             return a[0]
@@ -666,10 +643,10 @@ class TestRequestLocalCache(unittest.TestCase):
         req = DummyRequest()
         self.assertEqual(foo(req), 1)
         self.assertEqual(len(req.finished_callbacks), 1)
-        cache.set(req, 8)
+        foo.cache.set(req, 8)
         self.assertEqual(foo(req), 8)
         self.assertEqual(len(req.finished_callbacks), 1)
-        self.assertEqual(cache.get(req), 8)
+        self.assertEqual(foo.cache.get(req), 8)
 
     def test_get_works(self):
         cache = self._makeOne()
@@ -678,6 +655,47 @@ class TestRequestLocalCache(unittest.TestCase):
         cache.set(req, 2)
         self.assertIs(cache.get(req), 2)
 
+    def test_creator_in_constructor(self):
+
+        def foo(request):
+            return 8
+
+        cache = self._makeOne(foo)
+        req = DummyRequest()
+        result = cache.get_or_create(req)
+        self.assertEqual(result, 8)
+
+    def test_decorator_overrides_creator(self):
+
+        def foo(request):  # pragma: no cover
+            raise AssertionError
+
+        cache = self._makeOne(foo)
+
+        @cache
+        def bar(request):
+            return 8
+
+        req = DummyRequest()
+        result = cache.get_or_create(req)
+        self.assertEqual(result, 8)
+
+    def test_get_or_create_overrides_creator(self):
+        cache = self._makeOne()
+
+        @cache
+        def foo(request):  # pragma: no cover
+            raise AssertionError
+
+        req = DummyRequest()
+        result = cache.get_or_create(req, lambda r: 8)
+        self.assertEqual(result, 8)
+
+    def test_get_or_create_with_no_creator(self):
+        cache = self._makeOne()
+        req = DummyRequest()
+        self.assertRaises(ValueError, cache.get_or_create, req)
+
 
 class Dummy(object):
     pass