Merge lp:~lifeless/launchpad/cachedproperty into lp:launchpad

[Robert Collins]

Automatically clear cachedproperties on SQLBase derived classes when storm invalidates an object.

Make invalidation of cached properties on SQLBase derived classes automatic. This addresses many of the risks associated with using them in the test suite (the server already discards all such objects between transactions), leaving just read-write-readback cases to handle as and when they occur.

[Michael Hudson-Doyle (mwhudson)]

Hi,

This looks good. I'm not sure what value there is in initializing _cached_properties in SQLBase.__init__ is -- seems to slightly increase coupling.

Generally, yay for safer-by-default.

Cheers,
mwh

review: Approve

[Jeroen T. Vermeulen (jtv)]

Is there or will there be a similar mechanism for pure Storm classes? Otherwise this adds a nasty pitfall to Storm migration.

[Robert Collins (lifeless)]

On Mon, Aug 16, 2010 at 3:51 PM, Jeroen T. Vermeulen <email address hidden> wrote:
> Is there or will there be a similar mechanism for pure Storm classes?  Otherwise this adds a nasty pitfall to Storm migration.

Its a two liner:

def __storm_invalidate(self):
    clear_cachedproperties(self)

SQLBase is storm to all intents and purposes these days.

=== modified file 'lib/canonical/cachedproperty.py'
--- lib/canonical/cachedproperty.py	2010-01-11 21:29:33 +0000
+++ lib/canonical/cachedproperty.py	2010-08-17 01:31:02 +0000
@@ -3,10 +3,24 @@
 
 """Cached properties for situations where a property is computed once and
 then returned each time it is asked for.
+
+The clear_cachedproperties function can be used to wipe the cache of properties
+from an instance.
 """
 
 __metaclass__ = type
 
+__all__ = [
+    'cache_property',
+    'cachedproperty',
+    'clear_cachedproperties',
+    'clear_property',
+    ]
+
+from zope.security.proxy import removeSecurityProxy
+
+from canonical.lazr.utils import safe_hasattr
+
 # XXX: JonathanLange 2010-01-11 bug=505731: Move this to lp.services.
 
 def cachedproperty(attrname_or_fn):
@@ -17,6 +31,10 @@
 
     If you don't provide a name, the mangled name of the property is used.
 
+    cachedproperty is not threadsafe - it should not be used on objects which
+    are shared across threads / external locking should be used on those
+    objects.
+
     >>> class CachedPropertyTest(object):
     ...
     ...     @cachedproperty('_foo_cache')
@@ -44,6 +62,10 @@
     69
     >>> cpt._bar_cached_value
     69
+    
+    Cached properties are listed on instances.
+    >>> sorted(cpt._cached_properties)
+    ['_bar_cached_value', '_foo_cache']
 
     """
     if isinstance(attrname_or_fn, basestring):
@@ -54,8 +76,125 @@
         attrname = '_%s_cached_value' % fn.__name__
         return CachedProperty(attrname, fn)
 
+def cache_property(instance, attrname, value):
+    """Cache value on instance as attrname.
+    
+    instance._cached_properties is updated with attrname.
+
+        >>> class CachedPropertyTest(object):
+        ...
+        ...     @cachedproperty('_foo_cache')
+        ...     def foo(self):
+        ...         return 23
+        ...
+        >>> instance = CachedPropertyTest()
+        >>> cache_property(instance, '_foo_cache', 42)
+        >>> instance.foo
+        42
+        >>> instance._cached_properties
+        ['_foo_cache']
+
+    Caching a new value does not duplicate the cache keys.
+
+        >>> cache_property(instance, '_foo_cache', 84)
+        >>> instance._cached_properties
+        ['_foo_cache']
+
+    And does update the cached value.
+
+        >>> instance.foo
+        84
+    """
+    naked_instance = removeSecurityProxy(instance)
+    clear_property(naked_instance, attrname)
+    setattr(naked_instance, attrname, value)
+    cached_properties = getattr(naked_instance, '_cached_properties', [])
+    cached_properties.append(attrname)
+    naked_instance._cached_properties = cached_properties
+
+
+def clear_property(instance, attrname):
+    """Remove a cached attribute from instance.
+
+    The attribute name is removed from instance._cached_properties.
+
+    If the property is not cached, nothing happens.
+
+    :seealso clear_cachedproperties: For clearing all cached items at once.
+
+    >>> class CachedPropertyTest(object):
+    ...
+    ...     @cachedproperty('_foo_cache')
+    ...     def foo(self):
+    ...         return 23
+    ...
+    >>> instance = CachedPropertyTest()
+    >>> instance.foo
+    23
+    >>> clear_property(instance, '_foo_cache')
+    >>> instance._cached_properties
+    []
+    >>> is_cached(instance, '_foo_cache')
+    False
+    >>> clear_property(instance, '_foo_cache')
+    """
+    naked_instance = removeSecurityProxy(instance)
+    if not is_cached(naked_instance, attrname):
+        return
+    delattr(naked_instance, attrname)
+    naked_instance._cached_properties.remove(attrname)
+
+
+def clear_cachedproperties(instance):
+    """Clear cached properties from an object.
+    
+    >>> class CachedPropertyTest(object):
+    ...
+    ...     @cachedproperty('_foo_cache')
+    ...     def foo(self):
+    ...         return 23
+    ...
+    >>> instance = CachedPropertyTest()
+    >>> instance.foo
+    23
+    >>> instance._cached_properties
+    ['_foo_cache']
+    >>> clear_cachedproperties(instance)
+    >>> instance._cached_properties
+    []
+    >>> hasattr(instance, '_foo_cache')
+    False
+    """
+    naked_instance = removeSecurityProxy(instance)
+    cached_properties = getattr(naked_instance, '_cached_properties', [])
+    for property_name in cached_properties:
+        delattr(naked_instance, property_name)
+    naked_instance._cached_properties = []
+
+
+def is_cached(instance, attrname):
+    """Return True if attrname is cached on instance.
+
+    >>> class CachedPropertyTest(object):
+    ...
+    ...     @cachedproperty('_foo_cache')
+    ...     def foo(self):
+    ...         return 23
+    ...
+    >>> instance = CachedPropertyTest()
+    >>> instance.foo
+    23
+    >>> is_cached(instance, '_foo_cache')
+    True
+    >>> is_cached(instance, '_var_cache')
+    False
+    """
+    naked_instance = removeSecurityProxy(instance)
+    return safe_hasattr(naked_instance, attrname)
+
 
 class CachedPropertyForAttr:
+    """Curry a decorator to provide arguments to the CachedProperty."""
 
     def __init__(self, attrname):
         self.attrname = attrname
@@ -66,18 +205,20 @@
 
 class CachedProperty:
 
+    # Used to detect not-yet-cached properties.
+    sentinel = object()
+
     def __init__(self, attrname, fn):
         self.fn = fn
         self.attrname = attrname
-        self.marker = object()
 
     def __get__(self, inst, cls=None):
         if inst is None:
             return self
-        cachedresult = getattr(inst, self.attrname, self.marker)
-        if cachedresult is self.marker:
+        cachedresult = getattr(inst, self.attrname, CachedProperty.sentinel)
+        if cachedresult is CachedProperty.sentinel:
             result = self.fn(inst)
-            setattr(inst, self.attrname, result)
+            cache_property(inst, self.attrname, result)
             return result
         else:
             return cachedresult
 
=== modified file 'lib/canonical/database/sqlbase.py'
--- lib/canonical/database/sqlbase.py	2010-05-21 13:08:18 +0000
+++ lib/canonical/database/sqlbase.py	2010-08-17 01:31:02 +0000
@@ -30,6 +30,7 @@
 
 from lazr.restful.interfaces import IRepresentationCache
 
+from canonical.cachedproperty import clear_cachedproperties
 from canonical.config import config, dbconfig
 from canonical.database.interfaces import ISQLBase
 
@@ -175,6 +176,9 @@
         correct master Store.
         """
         from canonical.launchpad.interfaces import IMasterStore
+        # Make it simple to write dumb-invalidators - initialised
+        # _cached_properties to a valid list rather than just-in-time creation.
+        self._cached_properties = []
         store = IMasterStore(self.__class__)
 
         # The constructor will fail if objects from a different Store
@@ -253,6 +257,14 @@
         cache = getUtility(IRepresentationCache)
         cache.delete(self)
 
+    def __storm_invalidated__(self):
+        """Flush cached properties."""
+        # Note this is not directly tested; but the entire test suite blows up
+        # awesomely if its broken : its entirely unclear where tests for this
+        # should be -- RBC 20100816.
+        clear_cachedproperties(self)
+
+
 alreadyInstalledMsg = ("A ZopelessTransactionManager with these settings is "
 "already installed.  This is probably caused by calling initZopeless twice.")
 
 
=== modified file 'lib/canonical/launchpad/database/librarian.py'
--- lib/canonical/launchpad/database/librarian.py	2010-07-06 16:12:46 +0000
+++ lib/canonical/launchpad/database/librarian.py	2010-08-17 01:31:02 +0000
@@ -194,6 +194,7 @@
 
     def __storm_invalidated__(self):
         """Make sure that the file is closed across transaction boundary."""
+        super(LibraryFileAlias, self).__storm_invalidated__()
         self.close()
 
 
 
=== modified file 'lib/lp/registry/doc/teammembership.txt'
--- lib/lp/registry/doc/teammembership.txt	2010-07-14 15:28:42 +0000
+++ lib/lp/registry/doc/teammembership.txt	2010-08-17 01:31:02 +0000
@@ -1007,7 +1007,8 @@
     >>> from canonical.launchpad.interfaces import IMasterObject
     >>> IMasterObject(bad_user.account).status = AccountStatus.SUSPENDED
     >>> IMasterObject(bad_user.preferredemail).status = EmailAddressStatus.OLD
-    >>> removeSecurityProxy(bad_user)._preferredemail_cached = None
+    >>> from canonical.cachedproperty import clear_property
+    >>> clear_property(removeSecurityProxy(bad_user), '_preferredemail_cached')
     >>> transaction.commit()
 
     >>> [m.displayname for m in t3.allmembers]
 
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2010-08-11 02:17:14 +0000
+++ lib/lp/registry/model/distribution.py	2010-08-17 01:31:02 +0000
@@ -19,7 +19,7 @@
 from zope.interface import alsoProvides, implements
 
 from lp.archivepublisher.debversion import Version
-from canonical.cachedproperty import cachedproperty
+from canonical.cachedproperty import cachedproperty, clear_property
 from canonical.database.constants import UTC_NOW
 
 from canonical.database.datetimecol import UtcDateTimeCol
@@ -1617,8 +1617,9 @@
             # This driver is a release manager.
             series.driver = owner
 
-        if safe_hasattr(self, '_cached_series'):
-            del self._cached_series
+        # May wish to add this to the series rather than clearing the cache --
+        # RBC 20100816.
+        clear_property(self, '_cached_series')
         return series
 
     @property
 
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-08-13 08:26:15 +0000
+++ lib/lp/registry/model/person.py	2010-08-17 01:31:02 +0000
@@ -59,7 +59,7 @@
 from canonical.database.sqlbase import (
     cursor, quote, quote_like, sqlvalues, SQLBase)
 
-from canonical.cachedproperty import cachedproperty
+from canonical.cachedproperty import cachedproperty, cache_property, clear_property
 
 from canonical.lazr.utils import get_current_browser_request, safe_hasattr
 
@@ -390,13 +390,12 @@
 
         Order them by name if necessary.
         """
-        self._languages_cache = sorted(
-            languages, key=attrgetter('englishname'))
+        cache_property(self, '_languages_cache', sorted(
+            languages, key=attrgetter('englishname')))
 
     def deleteLanguagesCache(self):
         """Delete this person's cached languages, if it exists."""
-        if safe_hasattr(self, '_languages_cache'):
-            del self._languages_cache
+        clear_property(self, '_languages_cache')
 
     def addLanguage(self, language):
         """See `IPerson`."""
@@ -1462,7 +1461,7 @@
             include_teams=include_teams)
         person_list = []
         for person, email in result:
-            person._preferredemail_cached = email
+            cache_property(person, '_preferredemail_cached', email)
             person_list.append(person)
         return person_list
 
@@ -1737,7 +1736,7 @@
         self.account_status = AccountStatus.DEACTIVATED
         self.account_status_comment = comment
         IMasterObject(self.preferredemail).status = EmailAddressStatus.NEW
-        self._preferredemail_cached = None
+        clear_property(self, '_preferredemail_cached')
         base_new_name = self.name + '-deactivatedaccount'
         self.name = self._ensureNewName(base_new_name)
 
@@ -2070,7 +2069,7 @@
         if email_address is not None:
             email_address.status = EmailAddressStatus.VALIDATED
             email_address.syncUpdate()
-        self._preferredemail_cached = None
+        clear_property(self, '_preferredemail_cached')
 
     def setPreferredEmail(self, email):
         """See `IPerson`."""
@@ -2107,7 +2106,7 @@
         IMasterObject(email).syncUpdate()
 
         # Now we update our cache of the preferredemail.
-        self._preferredemail_cached = email
+        cache_property(self, '_preferredemail_cached', email)
 
     @cachedproperty('_preferredemail_cached')
     def preferredemail(self):
@@ -2622,7 +2621,8 @@
                 # Populate the previously empty 'preferredemail' cached
                 # property, so the Person record is up-to-date.
                 if master_email.status == EmailAddressStatus.PREFERRED:
-                    account_person._preferredemail_cached = master_email
+                    cache_property(account_person, '_preferredemail_cached',
+                        master_email)
                 return account_person
             # There is no associated `Person` to the email `Account`.
             # This is probably because the account was created externally
 
=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2010-08-06 14:49:35 +0000
+++ lib/lp/registry/model/product.py	2010-08-17 01:31:02 +0000
@@ -509,9 +509,8 @@
 
     def __storm_invalidated__(self):
         """Clear cached non-storm attributes when the transaction ends."""
+        super(Product, self).__storm_invalidated__()
         self._cached_licenses = None
-        if safe_hasattr(self, '_commercial_subscription_cached'):
-            del self._commercial_subscription_cached
 
     def _getLicenses(self):
         """Get the licenses as a tuple."""
 
=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py	2010-08-06 09:32:02 +0000
+++ lib/lp/translations/model/potemplate.py	2010-08-17 01:31:02 +0000
@@ -185,6 +185,7 @@
     _uses_english_msgids = None
 
     def __storm_invalidated__(self):
+        super(POTemplate, self).__storm_invalidated__()
         self.clearPOFileCache()
         self._uses_english_msgids = None
 
 
=== modified file 'lib/lp/translations/model/potmsgset.py'
--- lib/lp/translations/model/potmsgset.py	2010-08-06 06:51:37 +0000
+++ lib/lp/translations/model/potmsgset.py	2010-08-17 01:31:02 +0000
@@ -103,6 +103,7 @@
     credits_message_ids = credits_message_info.keys()
 
     def __storm_invalidated__(self):
+        super(POTMsgSet, self).__storm_invalidated__()
         self._cached_singular_text = None
         self._cached_uses_english_msgids = None
 
@@ -157,6 +158,7 @@
     @property
     def uses_english_msgids(self):
         """See `IPOTMsgSet`."""
+        # TODO: convert to cachedproperty, it will be simpler.
         if self._cached_uses_english_msgids is not None:
             return self._cached_uses_english_msgids
 
@@ -181,6 +183,7 @@
     @property
     def singular_text(self):
         """See `IPOTMsgSet`."""
+        # TODO: convert to cachedproperty, it will be simpler.
         if self._cached_singular_text is not None:
             return self._cached_singular_text