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

[Robert Collins]

Reduce the query count for team/participants to 11 from hundreds for ubuntu-dev.

Make the /participation API make a constant number of DB calls, rather than scaling per-team-member found.

This change has a few key components.

First, the existing allmembers attribute is perserved unaltered; rather a new all_members_prepopulated one is created and exported, so that the API fix doesn't cause unnecessary DB work for other callers that don't need all the attributes.

A common helper, _all_members is created on Person, which can prepopulate various attributes.

Secondly, all the attributes that are exported as fields, which the API wants to examine, are prepopulated, bringing the query count down to 11 for a team with 3 members (from nearly 40 - and that 11 is now constant in tests).

The prepopulation is done via cachedproperty decorators, which should be totally fine for webservice and API requests but does introduce a risk that other scripts which acccess these properties, and read-them-back-after-changing-the-originating-data will break, so when QA'ing a test run of all the registry related cronscripts is probably wise, though we have no particular data to suggest that this is needed.

I haven't run a full test suite yet, I will be doing so to find out if there are any side effects which will need fixing.

All that said, I think this is a more robust approach than having a separate CachedPersonDecorator and returning that - any changes to person would need more work in that case, where here it is built in.

[Curtis Hovey (sinzui)]

I am very pleased to see this enhancement. I expressed concerns that the @cachedproperty on is_valid_person and archive may cause issues during updated and subsequent displays of the person. eg, A user chooses to deactivate his profile, then the page loads and says he is still active, but reloading the page shows the page is deactivated. I am speculating though.

I think this is good to land. We will be prepared to address @cachedproperties as we do with Person.preferredemail, where the mutator function delete the _*_cached property to clear the state.

review: Approve (code)

[William Grant (wgrant)]

I think all POSTs redirect before displaying the subsequent page, so that should be fine. However, I'm very suspicious of widely deploying @cachedproperty... I'd be much happier if it was populated only by the explicit prejoin, and operated uncached in the normal case.

[Robert Collins (lifeless)]

On Tue, Aug 10, 2010 at 8:59 PM, William Grant <email address hidden> wrote:
> I think all POSTs redirect before displaying the subsequent page, so that should be fine. However, I'm very suspicious of widely deploying @cachedproperty... I'd be much happier if it was populated only by the explicit prejoin, and operated uncached in the normal case.

We /need/ a strategy for doing non-collection derived data in the
model, or we'll be playing 'omfg this is ugly shit', forever - and
never getting things fast.

Most of our pages only access 3 or 4 linked collections of objects,
but they generally do well over a hundred queries: this is
pathological!

I think its better that we experiment and find the problems and fix
them, than push back on actually getting what we need in place by
making the problem bigger than it is: cachedproperty isn't ideal, but
its a very small amount of work to use it.

-Rob

[Robert Collins (lifeless)]

This needs an incremental review.

Please ignore the cachedproperty.py changes other than those in rev 11324 - they are in my separate, approved, lp:~lifeless/launchpad/cachedproperty branch - that I started after this review, but merged in because it makes the branch cleaner and leaner.

You could pull that branch and merge this into it to see the incremental changes.

I'm particularly interested in the removeSecurityProxy calls in cachedproperty.py: I think they are appropriate and needed, but perhaps not?

The change in 11323 to invalidate the cache on Person is definitely needed and an expected consequence of more model object caching; taste wise we probably want to iterate towards an interface where the cache description ('_archive_cached') matches the cache attribute ('.archive') - but I think that that is essentially polish, not a prerequisite.

[Henning Eggers (henninge)]

Hi Robert,
thanks for all this work. We talked about the use of removeSecurityProxy in cachedproperty.py. It has to be clear that these are low-level functions that don't care about security. Security has to be considered as each call site. So this is mainly a documentation issue.

Please use "naked_instance" in the functions, though, to be clear about it.

Also, we agreed that you file a tech-debt bug about moving the code to lp.services and creating an extra doctest file instead of tests in docstring. Adding a unittest for extra points. ;-)

We will have to watch how these functions will be used in the future.

Cheers,
Henning

review: Approve (code)

=== modified file 'lib/canonical/cachedproperty.py'
--- lib/canonical/cachedproperty.py	2010-01-11 21:29:33 +0000
+++ lib/canonical/cachedproperty.py	2010-08-17 02:31:51 +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 02:31:51 +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 02:31:51 +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 02:31:51 +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/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2010-08-03 08:49:19 +0000
+++ lib/lp/registry/interfaces/person.py	2010-08-17 02:31:51 +0000
@@ -1231,7 +1231,7 @@
     all_member_count = Attribute(
         "The total number of real people who are members of this team, "
         "including subteams.")
-    allmembers = exported(
+    all_members_prepopulated = exported(
         doNotSnapshot(
             CollectionField(
                 title=_("All participants of this team."),
@@ -1243,6 +1243,8 @@
                     "IPerson.inTeam()."),
                 value_type=Reference(schema=Interface))),
         exported_as='participants')
+    allmembers = doNotSnapshot(
+        Attribute("List of all members, without checking karma etc."))
     approvedmembers = doNotSnapshot(
         Attribute("List of members with APPROVED status"))
     deactivated_member_count = Attribute("Number of deactivated members")
@@ -1322,14 +1324,17 @@
             center_lat, and center_lng
         """
 
-    def getMembersWithPreferredEmails(include_teams=False):
+    def getMembersWithPreferredEmails():
         """Returns a result set of persons with precached addresses.
 
         Persons or teams without preferred email addresses are not included.
         """
 
-    def getMembersWithPreferredEmailsCount(include_teams=False):
-        """Returns the count of persons/teams with preferred emails."""
+    def getMembersWithPreferredEmailsCount():
+        """Returns the count of persons/teams with preferred emails.
+        
+        See also getMembersWithPreferredEmails.
+        """
 
     def getDirectAdministrators():
         """Return this team's administrators.
@@ -2123,9 +2128,16 @@
 
 
 # Fix value_type.schema of IPersonViewRestricted attributes.
-for name in ['allmembers', 'activemembers', 'adminmembers', 'proposedmembers',
-             'invited_members', 'deactivatedmembers', 'expiredmembers',
-             'unmapped_participants']:
+for name in [
+    'all_members_prepopulated',
+    'activemembers',
+    'adminmembers',
+    'proposedmembers',
+    'invited_members',
+    'deactivatedmembers',
+    'expiredmembers',
+    'unmapped_participants',
+    ]:
     IPersonViewRestricted[name].value_type.schema = IPerson
 
 IPersonPublic['sub_teams'].value_type.schema = ITeam

=== 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 02:31:51 +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 02:31:51 +0000
@@ -48,7 +48,9 @@
     StringCol)
 from sqlobject.sqlbuilder import AND, OR, SQLConstant
 from storm.store import EmptyResultSet, Store
-from storm.expr import And, In, Join, LeftJoin, Lower, Not, Or, SQL
+from storm.expr import (
+    Alias, And, Exists, In, Join, LeftJoin, Lower, Min, Not, Or, Select, SQL,
+    )
 from storm.info import ClassAlias
 
 from canonical.config import config
@@ -59,10 +61,11 @@
 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
 
+from canonical.launchpad.components.decoratedresultset import DecoratedResultSet
 from canonical.launchpad.database.account import Account, AccountPassword
 from canonical.launchpad.interfaces.account import AccountSuspendedError
 from lp.bugs.model.bugtarget import HasBugsBase
@@ -390,13 +393,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`."""
@@ -1012,9 +1014,10 @@
         result = result.order_by(KarmaCategory.title)
         return [karma_cache for (karma_cache, category) in result]
 
-    @property
+    @cachedproperty('_karma_cached')
     def karma(self):
         """See `IPerson`."""
+        # May also be loaded from _all_members
         cache = KarmaTotalCache.selectOneBy(person=self)
         if cache is None:
             # Newly created accounts may not be in the cache yet, meaning the
@@ -1032,7 +1035,7 @@
 
         return self.is_valid_person
 
-    @property
+    @cachedproperty('_is_valid_person_cached')
     def is_valid_person(self):
         """See `IPerson`."""
         if self.is_team:
@@ -1431,14 +1434,131 @@
     @property
     def allmembers(self):
         """See `IPerson`."""
-        query = """
-            Person.id = TeamParticipation.person AND
-            TeamParticipation.team = %s AND
-            TeamParticipation.person != %s
-            """ % sqlvalues(self.id, self.id)
-        return Person.select(query, clauseTables=['TeamParticipation'])
-
-    def _getMembersWithPreferredEmails(self, include_teams=False):
+        return self._all_members()
+
+    @property
+    def all_members_prepopulated(self):
+        """See `IPerson`."""
+        return self._all_members(need_karma=True, need_ubuntu_coc=True,
+            need_location=True, need_archive=True, need_preferred_email=True,
+            need_validity=True)
+
+    def _all_members(self, need_karma=False, need_ubuntu_coc=False,
+        need_location=False, need_archive=False, need_preferred_email=False,
+        need_validity=False):
+        """Lookup all members of the team with optional precaching.
+        
+        :param need_karma: The karma attribute will be cached.
+        :param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
+            cached.
+        :param need_location: The location attribute will be cached.
+        :param need_archive: The archive attribute will be cached.
+        :param need_preferred_email: The preferred email attribute will be
+            cached.
+        :param need_validity: The is_valid attribute will be cached.
+        """
+        # TODO: consolidate this with getMembersWithPreferredEmails.
+        #       The difference between the two is that
+        #       getMembersWithPreferredEmails includes self, which is arguably
+        #       wrong, but perhaps deliberate.
+        store = Store.of(self)
+        origin = [
+            Person,
+            Join(TeamParticipation, TeamParticipation.person == Person.id),
+            ]
+        conditions = And(
+            # Members of this team,
+            TeamParticipation.team == self.id,
+            # But not the team itself.
+            TeamParticipation.person != self.id)
+        columns = [Person]
+        if need_karma:
+            # New people have no karmatotalcache rows.
+            origin.append(
+                LeftJoin(KarmaTotalCache, KarmaTotalCache.person == Person.id))
+            columns.append(KarmaTotalCache)
+        if need_ubuntu_coc:
+            columns.append(Alias(Exists(Select(SignedCodeOfConduct,
+                AND(Person._is_ubuntu_coc_signer_condition(),
+                    SignedCodeOfConduct.ownerID == Person.id))),
+                name='is_ubuntu_coc_signer'))
+        if need_location:
+            # New people have no location rows
+            origin.append(
+                LeftJoin(PersonLocation, PersonLocation.person == Person.id))
+            columns.append(PersonLocation)
+        if need_archive:
+            # Not everyone has PPA's 
+            # It would be nice to cleanly expose the soyuz rules for this to avoid
+            # duplicating the relationships.
+            origin.append(
+                LeftJoin(Archive, Archive.owner == Person.id))
+            columns.append(Archive)
+            conditions = And(conditions,
+                Or(Archive.id == None, And(
+                Archive.id == Select(Min(Archive.id),
+                    Archive.owner == Person.id, Archive),
+                Archive.purpose == ArchivePurpose.PPA)))
+        if need_preferred_email:
+            # Teams don't have email.
+            origin.append(
+                LeftJoin(EmailAddress, EmailAddress.person == Person.id))
+            columns.append(EmailAddress)
+            conditions = And(conditions,
+                Or(EmailAddress.status == None,
+                    EmailAddress.status == EmailAddressStatus.PREFERRED))
+        if need_validity:
+            # May find invalid persons
+            origin.append(
+                LeftJoin(ValidPersonCache, ValidPersonCache.id == Person.id))
+            columns.append(ValidPersonCache)
+        if len(columns) == 1:
+            columns = columns[0]
+            # Return a simple ResultSet
+            return store.using(*origin).find(columns, conditions)
+        # Adapt the result into a cached Person.
+        columns = tuple(columns)
+        raw_result = store.using(*origin).find(columns, conditions)
+        def prepopulate_person(row):
+            result = row[0]
+            index = 1
+            #-- karma caching
+            if need_karma:
+                karma = row[index]
+                index += 1
+                if karma is None:
+                    karma_total = 0
+                else:
+                    karma_total = karma.karma_total
+                cache_property(result, '_karma_cached', karma_total)
+            #-- ubuntu code of conduct signer status caching.
+            if need_ubuntu_coc:
+                signed = row[index]
+                index += 1
+                cache_property(result, '_is_ubuntu_coc_signer_cached', signed)
+            #-- location caching
+            if need_location:
+                location = row[index]
+                index += 1
+                cache_property(result, '_location', location)
+            #-- archive caching
+            if need_archive:
+                archive = row[index]
+                index += 1
+                cache_property(result, '_archive_cached', archive)
+            #-- preferred email caching
+            if need_preferred_email:
+                email = row[index]
+                index += 1
+                cache_property(result, '_preferredemail_cached', email)
+            if need_validity:
+                valid = row[index] is not None
+                index += 1
+                cache_property(result, '_is_valid_person_cached', valid)
+            return result
+        return DecoratedResultSet(raw_result, result_decorator=prepopulate_person)
+
+    def _getMembersWithPreferredEmails(self):
         """Helper method for public getMembersWithPreferredEmails.
 
         We can't return the preferred email address directly to the
@@ -1456,20 +1576,18 @@
             EmailAddress.status == EmailAddressStatus.PREFERRED)
         return store.using(*origin).find((Person, EmailAddress), conditions)
 
-    def getMembersWithPreferredEmails(self, include_teams=False):
+    def getMembersWithPreferredEmails(self):
         """See `IPerson`."""
-        result = self._getMembersWithPreferredEmails(
-            include_teams=include_teams)
+        result = self._getMembersWithPreferredEmails()
         person_list = []
         for person, email in result:
-            person._preferredemail_cached = email
+            cache_property(person, '_preferredemail_cached', email)
             person_list.append(person)
         return person_list
 
-    def getMembersWithPreferredEmailsCount(self, include_teams=False):
+    def getMembersWithPreferredEmailsCount(self):
         """See `IPerson`."""
-        result = self._getMembersWithPreferredEmails(
-            include_teams=include_teams)
+        result = self._getMembersWithPreferredEmails()
         return result.count()
 
     @property
@@ -1737,7 +1855,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 +2188,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 +2225,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):
@@ -2274,17 +2392,23 @@
             distribution.main_archive, self)
         return permissions.count() > 0
 
-    @cachedproperty
+    @cachedproperty('_is_ubuntu_coc_signer_cached')
     def is_ubuntu_coc_signer(self):
         """See `IPerson`."""
+        # Also assigned to by self._all_members.
+        store = Store.of(self)
+        query = AND(SignedCodeOfConduct.ownerID == self.id,
+            Person._is_ubuntu_coc_signer_condition())
+        # TODO: Using exists would be faster than count().
+        return bool(store.find(SignedCodeOfConduct, query).count())
+
+    @staticmethod
+    def _is_ubuntu_coc_signer_condition():
+        """Generate a Storm Expr for determing the coc signing status."""
         sigset = getUtility(ISignedCodeOfConductSet)
         lastdate = sigset.getLastAcceptedDate()
-
-        query = AND(SignedCodeOfConduct.q.active==True,
-                    SignedCodeOfConduct.q.ownerID==self.id,
-                    SignedCodeOfConduct.q.datecreated>=lastdate)
-
-        return bool(SignedCodeOfConduct.select(query).count())
+        return AND(SignedCodeOfConduct.active == True,
+            SignedCodeOfConduct.datecreated >= lastdate)
 
     @property
     def activesignatures(self):
@@ -2298,7 +2422,7 @@
         sCoC_util = getUtility(ISignedCodeOfConductSet)
         return sCoC_util.searchByUser(self.id, active=False)
 
-    @property
+    @cachedproperty('_archive_cached')
     def archive(self):
         """See `IPerson`."""
         return getUtility(IArchiveSet).getPPAOwnedByPerson(self)
@@ -2622,7 +2746,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 02:31:51 +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/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-08-13 07:48:54 +0000
+++ lib/lp/registry/tests/test_person.py	2010-08-17 02:31:51 +0000
@@ -1,12 +1,16 @@
 # Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+from __future__ import with_statement
+
 __metaclass__ = type
 
 from datetime import datetime
 import pytz
 import time
 
+from testtools.matchers import LessThan
+
 import transaction
 
 from zope.component import getUtility
@@ -26,6 +30,7 @@
     IPersonSet, ImmutableVisibilityError, NameAlreadyTaken,
     PersonCreationRationale, PersonVisibility)
 from canonical.launchpad.database import Bug
+from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from lp.registry.model.structuralsubscription import (
     StructuralSubscription)
 from lp.registry.model.karma import KarmaCategory
@@ -34,8 +39,12 @@
 from lp.bugs.interfaces.bugtask import IllegalRelatedBugTasksParams
 from lp.answers.model.answercontact import AnswerContact
 from lp.blueprints.model.specification import Specification
-from lp.testing import login_person, logout, TestCase, TestCaseWithFactory
+from lp.testing import (
+    login_person, logout, person_logged_in, TestCase, TestCaseWithFactory,
+    )
+from lp.testing.matchers import HasQueryCount
 from lp.testing.views import create_initialized_view
+from lp.testing._webservice import QueryCollector
 from lp.registry.interfaces.person import PrivatePersonLinkageError
 from canonical.testing.layers import DatabaseFunctionalLayer, reconnect_stores
 
@@ -191,7 +200,8 @@
 
     def test_person_snapshot(self):
         omitted = (
-            'activemembers', 'adminmembers', 'allmembers', 'approvedmembers',
+            'activemembers', 'adminmembers', 'allmembers',
+            'all_members_prepopulated',  'approvedmembers',
             'deactivatedmembers', 'expiredmembers', 'inactivemembers',
             'invited_members', 'member_memberships', 'pendingmembers',
             'proposedmembers', 'unmapped_participants',
@@ -553,3 +563,31 @@
         names = [entry['project'].name for entry in results]
         self.assertEqual(
             ['cc', 'bb', 'aa', 'dd', 'ee'], names)
+
+
+class TestAPIPartipication(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_participation_query_limit(self):
+        # A team with 3 members should only query once for all their
+        # attributes.
+        team = self.factory.makeTeam()
+        with person_logged_in(team.teamowner):
+            team.addMember(self.factory.makePerson(), team.teamowner)
+            team.addMember(self.factory.makePerson(), team.teamowner)
+            team.addMember(self.factory.makePerson(), team.teamowner)
+        webservice = LaunchpadWebServiceCaller()
+        collector = QueryCollector()
+        collector.register()
+        self.addCleanup(collector.unregister)
+        url = "/~%s/participants" % team.name
+        logout()
+        response = webservice.get(url, headers={'User-Agent':'AnonNeedsThis'})
+        self.assertEqual(response.status, 200,
+            "Got %d for url %r with response %r" % (
+            response.status, url, response.body))
+        # XXX: This number should really be 10, but see
+        # https://bugs.launchpad.net/storm/+bug/619017 which is adding 3
+        # queries to the test.
+        self.assertThat(collector, HasQueryCount(LessThan(13)))

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2010-08-05 13:23:52 +0000
+++ lib/lp/soyuz/model/archive.py	2010-08-17 02:31:51 +0000
@@ -25,6 +25,7 @@
 from lp.app.errors import NotFoundError
 from lp.archivepublisher.debversion import Version
 from lp.archiveuploader.utils import re_issource, re_isadeb
+from canonical.cachedproperty import clear_property
 from canonical.config import config
 from canonical.database.constants import UTC_NOW
 from canonical.database.datetimecol import UtcDateTimeCol
@@ -1753,8 +1754,12 @@
 
         # Signing-key for the default PPA is reused when it's already present.
         signing_key = None
-        if purpose == ArchivePurpose.PPA and owner.archive is not None:
-            signing_key = owner.archive.signing_key
+        if purpose == ArchivePurpose.PPA:
+            if owner.archive is not None:
+                signing_key = owner.archive.signing_key
+            else:
+                # owner.archive is a cached property and we've just cached it.
+                clear_property(owner, '_archive_cached')
 
         new_archive = Archive(
             owner=owner, distribution=distribution, name=name,
@@ -1806,6 +1811,7 @@
 
     def getPPAOwnedByPerson(self, person, name=None):
         """See `IArchiveSet`."""
+        # See Person._all_members which also directly queries this.
         store = Store.of(person)
         clause = [
             Archive.purpose == ArchivePurpose.PPA,

=== 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 02:31:51 +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 02:31:51 +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