Launchpad itself

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

  1. registry
  2. Merge into devel
Proposed by Robert Collins
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 11367
Proposed branch: lp:~lifeless/launchpad/registry
Merge into: lp:launchpad
Diff against target: 835 lines (+396/-56)
12 files modified
lib/canonical/cachedproperty.py (+145/-4)
lib/canonical/database/sqlbase.py (+12/-0)
lib/canonical/launchpad/database/librarian.py (+1/-0)
lib/lp/registry/doc/teammembership.txt (+2/-1)
lib/lp/registry/interfaces/person.py (+19/-7)
lib/lp/registry/model/distribution.py (+4/-3)
lib/lp/registry/model/person.py (+160/-35)
lib/lp/registry/model/product.py (+1/-2)
lib/lp/registry/tests/test_person.py (+40/-2)
lib/lp/soyuz/model/archive.py (+8/-2)
lib/lp/translations/model/potemplate.py (+1/-0)
lib/lp/translations/model/potmsgset.py (+3/-0)
To merge this branch: bzr merge lp:~lifeless/launchpad/registry
Reviewer Review Type Date Requested Status
Henning Eggers (community) code Approve
Curtis Hovey (community) code Approve
Review via email: mp+32067@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

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)
Revision history for this message
William Grant (wgrant) 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.

Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Robert Collins (lifeless) wrote :

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.

Revision history for this message
Henning Eggers (henninge) wrote :

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
33
4"""Cached properties for situations where a property is computed once and4"""Cached properties for situations where a property is computed once and
5then returned each time it is asked for.5then returned each time it is asked for.
6
7The clear_cachedproperties function can be used to wipe the cache of properties
8from an instance.
6"""9"""
710
8__metaclass__ = type11__metaclass__ = type
912
13__all__ = [
14 'cache_property',
15 'cachedproperty',
16 'clear_cachedproperties',
17 'clear_property',
18 ]
19
20from zope.security.proxy import removeSecurityProxy
21
22from canonical.lazr.utils import safe_hasattr
23
10# XXX: JonathanLange 2010-01-11 bug=505731: Move this to lp.services.24# XXX: JonathanLange 2010-01-11 bug=505731: Move this to lp.services.
1125
12def cachedproperty(attrname_or_fn):26def cachedproperty(attrname_or_fn):
@@ -17,6 +31,10 @@
1731
18 If you don't provide a name, the mangled name of the property is used.32 If you don't provide a name, the mangled name of the property is used.
1933
34 cachedproperty is not threadsafe - it should not be used on objects which
35 are shared across threads / external locking should be used on those
36 objects.
37
20 >>> class CachedPropertyTest(object):38 >>> class CachedPropertyTest(object):
21 ...39 ...
22 ... @cachedproperty('_foo_cache')40 ... @cachedproperty('_foo_cache')
@@ -44,6 +62,10 @@
44 6962 69
45 >>> cpt._bar_cached_value63 >>> cpt._bar_cached_value
46 6964 69
65
66 Cached properties are listed on instances.
67 >>> sorted(cpt._cached_properties)
68 ['_bar_cached_value', '_foo_cache']
4769
48 """70 """
49 if isinstance(attrname_or_fn, basestring):71 if isinstance(attrname_or_fn, basestring):
@@ -54,8 +76,125 @@
54 attrname = '_%s_cached_value' % fn.__name__76 attrname = '_%s_cached_value' % fn.__name__
55 return CachedProperty(attrname, fn)77 return CachedProperty(attrname, fn)
5678
79def cache_property(instance, attrname, value):
80 """Cache value on instance as attrname.
81
82 instance._cached_properties is updated with attrname.
83
84 >>> class CachedPropertyTest(object):
85 ...
86 ... @cachedproperty('_foo_cache')
87 ... def foo(self):
88 ... return 23
89 ...
90 >>> instance = CachedPropertyTest()
91 >>> cache_property(instance, '_foo_cache', 42)
92 >>> instance.foo
93 42
94 >>> instance._cached_properties
95 ['_foo_cache']
96
97 Caching a new value does not duplicate the cache keys.
98
99 >>> cache_property(instance, '_foo_cache', 84)
100 >>> instance._cached_properties
101 ['_foo_cache']
102
103 And does update the cached value.
104
105 >>> instance.foo
106 84
107 """
108 naked_instance = removeSecurityProxy(instance)
109 clear_property(naked_instance, attrname)
110 setattr(naked_instance, attrname, value)
111 cached_properties = getattr(naked_instance, '_cached_properties', [])
112 cached_properties.append(attrname)
113 naked_instance._cached_properties = cached_properties
114
115
116def clear_property(instance, attrname):
117 """Remove a cached attribute from instance.
118
119 The attribute name is removed from instance._cached_properties.
120
121 If the property is not cached, nothing happens.
122
123 :seealso clear_cachedproperties: For clearing all cached items at once.
124
125 >>> class CachedPropertyTest(object):
126 ...
127 ... @cachedproperty('_foo_cache')
128 ... def foo(self):
129 ... return 23
130 ...
131 >>> instance = CachedPropertyTest()
132 >>> instance.foo
133 23
134 >>> clear_property(instance, '_foo_cache')
135 >>> instance._cached_properties
136 []
137 >>> is_cached(instance, '_foo_cache')
138 False
139 >>> clear_property(instance, '_foo_cache')
140 """
141 naked_instance = removeSecurityProxy(instance)
142 if not is_cached(naked_instance, attrname):
143 return
144 delattr(naked_instance, attrname)
145 naked_instance._cached_properties.remove(attrname)
146
147
148def clear_cachedproperties(instance):
149 """Clear cached properties from an object.
150
151 >>> class CachedPropertyTest(object):
152 ...
153 ... @cachedproperty('_foo_cache')
154 ... def foo(self):
155 ... return 23
156 ...
157 >>> instance = CachedPropertyTest()
158 >>> instance.foo
159 23
160 >>> instance._cached_properties
161 ['_foo_cache']
162 >>> clear_cachedproperties(instance)
163 >>> instance._cached_properties
164 []
165 >>> hasattr(instance, '_foo_cache')
166 False
167 """
168 naked_instance = removeSecurityProxy(instance)
169 cached_properties = getattr(naked_instance, '_cached_properties', [])
170 for property_name in cached_properties:
171 delattr(naked_instance, property_name)
172 naked_instance._cached_properties = []
173
174
175def is_cached(instance, attrname):
176 """Return True if attrname is cached on instance.
177
178 >>> class CachedPropertyTest(object):
179 ...
180 ... @cachedproperty('_foo_cache')
181 ... def foo(self):
182 ... return 23
183 ...
184 >>> instance = CachedPropertyTest()
185 >>> instance.foo
186 23
187 >>> is_cached(instance, '_foo_cache')
188 True
189 >>> is_cached(instance, '_var_cache')
190 False
191 """
192 naked_instance = removeSecurityProxy(instance)
193 return safe_hasattr(naked_instance, attrname)
194
57195
58class CachedPropertyForAttr:196class CachedPropertyForAttr:
197 """Curry a decorator to provide arguments to the CachedProperty."""
59198
60 def __init__(self, attrname):199 def __init__(self, attrname):
61 self.attrname = attrname200 self.attrname = attrname
@@ -66,18 +205,20 @@
66205
67class CachedProperty:206class CachedProperty:
68207
208 # Used to detect not-yet-cached properties.
209 sentinel = object()
210
69 def __init__(self, attrname, fn):211 def __init__(self, attrname, fn):
70 self.fn = fn212 self.fn = fn
71 self.attrname = attrname213 self.attrname = attrname
72 self.marker = object()
73214
74 def __get__(self, inst, cls=None):215 def __get__(self, inst, cls=None):
75 if inst is None:216 if inst is None:
76 return self217 return self
77 cachedresult = getattr(inst, self.attrname, self.marker)218 cachedresult = getattr(inst, self.attrname, CachedProperty.sentinel)
78 if cachedresult is self.marker:219 if cachedresult is CachedProperty.sentinel:
79 result = self.fn(inst)220 result = self.fn(inst)
80 setattr(inst, self.attrname, result)221 cache_property(inst, self.attrname, result)
81 return result222 return result
82 else:223 else:
83 return cachedresult224 return cachedresult
84225
=== 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 @@
3030
31from lazr.restful.interfaces import IRepresentationCache31from lazr.restful.interfaces import IRepresentationCache
3232
33from canonical.cachedproperty import clear_cachedproperties
33from canonical.config import config, dbconfig34from canonical.config import config, dbconfig
34from canonical.database.interfaces import ISQLBase35from canonical.database.interfaces import ISQLBase
3536
@@ -175,6 +176,9 @@
175 correct master Store.176 correct master Store.
176 """177 """
177 from canonical.launchpad.interfaces import IMasterStore178 from canonical.launchpad.interfaces import IMasterStore
179 # Make it simple to write dumb-invalidators - initialised
180 # _cached_properties to a valid list rather than just-in-time creation.
181 self._cached_properties = []
178 store = IMasterStore(self.__class__)182 store = IMasterStore(self.__class__)
179183
180 # The constructor will fail if objects from a different Store184 # The constructor will fail if objects from a different Store
@@ -253,6 +257,14 @@
253 cache = getUtility(IRepresentationCache)257 cache = getUtility(IRepresentationCache)
254 cache.delete(self)258 cache.delete(self)
255259
260 def __storm_invalidated__(self):
261 """Flush cached properties."""
262 # Note this is not directly tested; but the entire test suite blows up
263 # awesomely if its broken : its entirely unclear where tests for this
264 # should be -- RBC 20100816.
265 clear_cachedproperties(self)
266
267
256alreadyInstalledMsg = ("A ZopelessTransactionManager with these settings is "268alreadyInstalledMsg = ("A ZopelessTransactionManager with these settings is "
257"already installed. This is probably caused by calling initZopeless twice.")269"already installed. This is probably caused by calling initZopeless twice.")
258270
259271
=== 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 @@
194194
195 def __storm_invalidated__(self):195 def __storm_invalidated__(self):
196 """Make sure that the file is closed across transaction boundary."""196 """Make sure that the file is closed across transaction boundary."""
197 super(LibraryFileAlias, self).__storm_invalidated__()
197 self.close()198 self.close()
198199
199200
200201
=== 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 @@
1007 >>> from canonical.launchpad.interfaces import IMasterObject1007 >>> from canonical.launchpad.interfaces import IMasterObject
1008 >>> IMasterObject(bad_user.account).status = AccountStatus.SUSPENDED1008 >>> IMasterObject(bad_user.account).status = AccountStatus.SUSPENDED
1009 >>> IMasterObject(bad_user.preferredemail).status = EmailAddressStatus.OLD1009 >>> IMasterObject(bad_user.preferredemail).status = EmailAddressStatus.OLD
1010 >>> removeSecurityProxy(bad_user)._preferredemail_cached = None1010 >>> from canonical.cachedproperty import clear_property
1011 >>> clear_property(removeSecurityProxy(bad_user), '_preferredemail_cached')
1011 >>> transaction.commit()1012 >>> transaction.commit()
10121013
1013 >>> [m.displayname for m in t3.allmembers]1014 >>> [m.displayname for m in t3.allmembers]
10141015
=== 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 @@
1231 all_member_count = Attribute(1231 all_member_count = Attribute(
1232 "The total number of real people who are members of this team, "1232 "The total number of real people who are members of this team, "
1233 "including subteams.")1233 "including subteams.")
1234 allmembers = exported(1234 all_members_prepopulated = exported(
1235 doNotSnapshot(1235 doNotSnapshot(
1236 CollectionField(1236 CollectionField(
1237 title=_("All participants of this team."),1237 title=_("All participants of this team."),
@@ -1243,6 +1243,8 @@
1243 "IPerson.inTeam()."),1243 "IPerson.inTeam()."),
1244 value_type=Reference(schema=Interface))),1244 value_type=Reference(schema=Interface))),
1245 exported_as='participants')1245 exported_as='participants')
1246 allmembers = doNotSnapshot(
1247 Attribute("List of all members, without checking karma etc."))
1246 approvedmembers = doNotSnapshot(1248 approvedmembers = doNotSnapshot(
1247 Attribute("List of members with APPROVED status"))1249 Attribute("List of members with APPROVED status"))
1248 deactivated_member_count = Attribute("Number of deactivated members")1250 deactivated_member_count = Attribute("Number of deactivated members")
@@ -1322,14 +1324,17 @@
1322 center_lat, and center_lng1324 center_lat, and center_lng
1323 """1325 """
13241326
1325 def getMembersWithPreferredEmails(include_teams=False):1327 def getMembersWithPreferredEmails():
1326 """Returns a result set of persons with precached addresses.1328 """Returns a result set of persons with precached addresses.
13271329
1328 Persons or teams without preferred email addresses are not included.1330 Persons or teams without preferred email addresses are not included.
1329 """1331 """
13301332
1331 def getMembersWithPreferredEmailsCount(include_teams=False):1333 def getMembersWithPreferredEmailsCount():
1332 """Returns the count of persons/teams with preferred emails."""1334 """Returns the count of persons/teams with preferred emails.
1335
1336 See also getMembersWithPreferredEmails.
1337 """
13331338
1334 def getDirectAdministrators():1339 def getDirectAdministrators():
1335 """Return this team's administrators.1340 """Return this team's administrators.
@@ -2123,9 +2128,16 @@
21232128
21242129
2125# Fix value_type.schema of IPersonViewRestricted attributes.2130# Fix value_type.schema of IPersonViewRestricted attributes.
2126for name in ['allmembers', 'activemembers', 'adminmembers', 'proposedmembers',2131for name in [
2127 'invited_members', 'deactivatedmembers', 'expiredmembers',2132 'all_members_prepopulated',
2128 'unmapped_participants']:2133 'activemembers',
2134 'adminmembers',
2135 'proposedmembers',
2136 'invited_members',
2137 'deactivatedmembers',
2138 'expiredmembers',
2139 'unmapped_participants',
2140 ]:
2129 IPersonViewRestricted[name].value_type.schema = IPerson2141 IPersonViewRestricted[name].value_type.schema = IPerson
21302142
2131IPersonPublic['sub_teams'].value_type.schema = ITeam2143IPersonPublic['sub_teams'].value_type.schema = ITeam
21322144
=== 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 @@
19from zope.interface import alsoProvides, implements19from zope.interface import alsoProvides, implements
2020
21from lp.archivepublisher.debversion import Version21from lp.archivepublisher.debversion import Version
22from canonical.cachedproperty import cachedproperty22from canonical.cachedproperty import cachedproperty, clear_property
23from canonical.database.constants import UTC_NOW23from canonical.database.constants import UTC_NOW
2424
25from canonical.database.datetimecol import UtcDateTimeCol25from canonical.database.datetimecol import UtcDateTimeCol
@@ -1617,8 +1617,9 @@
1617 # This driver is a release manager.1617 # This driver is a release manager.
1618 series.driver = owner1618 series.driver = owner
16191619
1620 if safe_hasattr(self, '_cached_series'):1620 # May wish to add this to the series rather than clearing the cache --
1621 del self._cached_series1621 # RBC 20100816.
1622 clear_property(self, '_cached_series')
1622 return series1623 return series
16231624
1624 @property1625 @property
16251626
=== 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 @@
48 StringCol)48 StringCol)
49from sqlobject.sqlbuilder import AND, OR, SQLConstant49from sqlobject.sqlbuilder import AND, OR, SQLConstant
50from storm.store import EmptyResultSet, Store50from storm.store import EmptyResultSet, Store
51from storm.expr import And, In, Join, LeftJoin, Lower, Not, Or, SQL51from storm.expr import (
52 Alias, And, Exists, In, Join, LeftJoin, Lower, Min, Not, Or, Select, SQL,
53 )
52from storm.info import ClassAlias54from storm.info import ClassAlias
5355
54from canonical.config import config56from canonical.config import config
@@ -59,10 +61,11 @@
59from canonical.database.sqlbase import (61from canonical.database.sqlbase import (
60 cursor, quote, quote_like, sqlvalues, SQLBase)62 cursor, quote, quote_like, sqlvalues, SQLBase)
6163
62from canonical.cachedproperty import cachedproperty64from canonical.cachedproperty import cachedproperty, cache_property, clear_property
6365
64from canonical.lazr.utils import get_current_browser_request, safe_hasattr66from canonical.lazr.utils import get_current_browser_request, safe_hasattr
6567
68from canonical.launchpad.components.decoratedresultset import DecoratedResultSet
66from canonical.launchpad.database.account import Account, AccountPassword69from canonical.launchpad.database.account import Account, AccountPassword
67from canonical.launchpad.interfaces.account import AccountSuspendedError70from canonical.launchpad.interfaces.account import AccountSuspendedError
68from lp.bugs.model.bugtarget import HasBugsBase71from lp.bugs.model.bugtarget import HasBugsBase
@@ -390,13 +393,12 @@
390393
391 Order them by name if necessary.394 Order them by name if necessary.
392 """395 """
393 self._languages_cache = sorted(396 cache_property(self, '_languages_cache', sorted(
394 languages, key=attrgetter('englishname'))397 languages, key=attrgetter('englishname')))
395398
396 def deleteLanguagesCache(self):399 def deleteLanguagesCache(self):
397 """Delete this person's cached languages, if it exists."""400 """Delete this person's cached languages, if it exists."""
398 if safe_hasattr(self, '_languages_cache'):401 clear_property(self, '_languages_cache')
399 del self._languages_cache
400402
401 def addLanguage(self, language):403 def addLanguage(self, language):
402 """See `IPerson`."""404 """See `IPerson`."""
@@ -1012,9 +1014,10 @@
1012 result = result.order_by(KarmaCategory.title)1014 result = result.order_by(KarmaCategory.title)
1013 return [karma_cache for (karma_cache, category) in result]1015 return [karma_cache for (karma_cache, category) in result]
10141016
1015 @property1017 @cachedproperty('_karma_cached')
1016 def karma(self):1018 def karma(self):
1017 """See `IPerson`."""1019 """See `IPerson`."""
1020 # May also be loaded from _all_members
1018 cache = KarmaTotalCache.selectOneBy(person=self)1021 cache = KarmaTotalCache.selectOneBy(person=self)
1019 if cache is None:1022 if cache is None:
1020 # Newly created accounts may not be in the cache yet, meaning the1023 # Newly created accounts may not be in the cache yet, meaning the
@@ -1032,7 +1035,7 @@
10321035
1033 return self.is_valid_person1036 return self.is_valid_person
10341037
1035 @property1038 @cachedproperty('_is_valid_person_cached')
1036 def is_valid_person(self):1039 def is_valid_person(self):
1037 """See `IPerson`."""1040 """See `IPerson`."""
1038 if self.is_team:1041 if self.is_team:
@@ -1431,14 +1434,131 @@
1431 @property1434 @property
1432 def allmembers(self):1435 def allmembers(self):
1433 """See `IPerson`."""1436 """See `IPerson`."""
1434 query = """1437 return self._all_members()
1435 Person.id = TeamParticipation.person AND1438
1436 TeamParticipation.team = %s AND1439 @property
1437 TeamParticipation.person != %s1440 def all_members_prepopulated(self):
1438 """ % sqlvalues(self.id, self.id)1441 """See `IPerson`."""
1439 return Person.select(query, clauseTables=['TeamParticipation'])1442 return self._all_members(need_karma=True, need_ubuntu_coc=True,
14401443 need_location=True, need_archive=True, need_preferred_email=True,
1441 def _getMembersWithPreferredEmails(self, include_teams=False):1444 need_validity=True)
1445
1446 def _all_members(self, need_karma=False, need_ubuntu_coc=False,
1447 need_location=False, need_archive=False, need_preferred_email=False,
1448 need_validity=False):
1449 """Lookup all members of the team with optional precaching.
1450
1451 :param need_karma: The karma attribute will be cached.
1452 :param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
1453 cached.
1454 :param need_location: The location attribute will be cached.
1455 :param need_archive: The archive attribute will be cached.
1456 :param need_preferred_email: The preferred email attribute will be
1457 cached.
1458 :param need_validity: The is_valid attribute will be cached.
1459 """
1460 # TODO: consolidate this with getMembersWithPreferredEmails.
1461 # The difference between the two is that
1462 # getMembersWithPreferredEmails includes self, which is arguably
1463 # wrong, but perhaps deliberate.
1464 store = Store.of(self)
1465 origin = [
1466 Person,
1467 Join(TeamParticipation, TeamParticipation.person == Person.id),
1468 ]
1469 conditions = And(
1470 # Members of this team,
1471 TeamParticipation.team == self.id,
1472 # But not the team itself.
1473 TeamParticipation.person != self.id)
1474 columns = [Person]
1475 if need_karma:
1476 # New people have no karmatotalcache rows.
1477 origin.append(
1478 LeftJoin(KarmaTotalCache, KarmaTotalCache.person == Person.id))
1479 columns.append(KarmaTotalCache)
1480 if need_ubuntu_coc:
1481 columns.append(Alias(Exists(Select(SignedCodeOfConduct,
1482 AND(Person._is_ubuntu_coc_signer_condition(),
1483 SignedCodeOfConduct.ownerID == Person.id))),
1484 name='is_ubuntu_coc_signer'))
1485 if need_location:
1486 # New people have no location rows
1487 origin.append(
1488 LeftJoin(PersonLocation, PersonLocation.person == Person.id))
1489 columns.append(PersonLocation)
1490 if need_archive:
1491 # Not everyone has PPA's
1492 # It would be nice to cleanly expose the soyuz rules for this to avoid
1493 # duplicating the relationships.
1494 origin.append(
1495 LeftJoin(Archive, Archive.owner == Person.id))
1496 columns.append(Archive)
1497 conditions = And(conditions,
1498 Or(Archive.id == None, And(
1499 Archive.id == Select(Min(Archive.id),
1500 Archive.owner == Person.id, Archive),
1501 Archive.purpose == ArchivePurpose.PPA)))
1502 if need_preferred_email:
1503 # Teams don't have email.
1504 origin.append(
1505 LeftJoin(EmailAddress, EmailAddress.person == Person.id))
1506 columns.append(EmailAddress)
1507 conditions = And(conditions,
1508 Or(EmailAddress.status == None,
1509 EmailAddress.status == EmailAddressStatus.PREFERRED))
1510 if need_validity:
1511 # May find invalid persons
1512 origin.append(
1513 LeftJoin(ValidPersonCache, ValidPersonCache.id == Person.id))
1514 columns.append(ValidPersonCache)
1515 if len(columns) == 1:
1516 columns = columns[0]
1517 # Return a simple ResultSet
1518 return store.using(*origin).find(columns, conditions)
1519 # Adapt the result into a cached Person.
1520 columns = tuple(columns)
1521 raw_result = store.using(*origin).find(columns, conditions)
1522 def prepopulate_person(row):
1523 result = row[0]
1524 index = 1
1525 #-- karma caching
1526 if need_karma:
1527 karma = row[index]
1528 index += 1
1529 if karma is None:
1530 karma_total = 0
1531 else:
1532 karma_total = karma.karma_total
1533 cache_property(result, '_karma_cached', karma_total)
1534 #-- ubuntu code of conduct signer status caching.
1535 if need_ubuntu_coc:
1536 signed = row[index]
1537 index += 1
1538 cache_property(result, '_is_ubuntu_coc_signer_cached', signed)
1539 #-- location caching
1540 if need_location:
1541 location = row[index]
1542 index += 1
1543 cache_property(result, '_location', location)
1544 #-- archive caching
1545 if need_archive:
1546 archive = row[index]
1547 index += 1
1548 cache_property(result, '_archive_cached', archive)
1549 #-- preferred email caching
1550 if need_preferred_email:
1551 email = row[index]
1552 index += 1
1553 cache_property(result, '_preferredemail_cached', email)
1554 if need_validity:
1555 valid = row[index] is not None
1556 index += 1
1557 cache_property(result, '_is_valid_person_cached', valid)
1558 return result
1559 return DecoratedResultSet(raw_result, result_decorator=prepopulate_person)
1560
1561 def _getMembersWithPreferredEmails(self):
1442 """Helper method for public getMembersWithPreferredEmails.1562 """Helper method for public getMembersWithPreferredEmails.
14431563
1444 We can't return the preferred email address directly to the1564 We can't return the preferred email address directly to the
@@ -1456,20 +1576,18 @@
1456 EmailAddress.status == EmailAddressStatus.PREFERRED)1576 EmailAddress.status == EmailAddressStatus.PREFERRED)
1457 return store.using(*origin).find((Person, EmailAddress), conditions)1577 return store.using(*origin).find((Person, EmailAddress), conditions)
14581578
1459 def getMembersWithPreferredEmails(self, include_teams=False):1579 def getMembersWithPreferredEmails(self):
1460 """See `IPerson`."""1580 """See `IPerson`."""
1461 result = self._getMembersWithPreferredEmails(1581 result = self._getMembersWithPreferredEmails()
1462 include_teams=include_teams)
1463 person_list = []1582 person_list = []
1464 for person, email in result:1583 for person, email in result:
1465 person._preferredemail_cached = email1584 cache_property(person, '_preferredemail_cached', email)
1466 person_list.append(person)1585 person_list.append(person)
1467 return person_list1586 return person_list
14681587
1469 def getMembersWithPreferredEmailsCount(self, include_teams=False):1588 def getMembersWithPreferredEmailsCount(self):
1470 """See `IPerson`."""1589 """See `IPerson`."""
1471 result = self._getMembersWithPreferredEmails(1590 result = self._getMembersWithPreferredEmails()
1472 include_teams=include_teams)
1473 return result.count()1591 return result.count()
14741592
1475 @property1593 @property
@@ -1737,7 +1855,7 @@
1737 self.account_status = AccountStatus.DEACTIVATED1855 self.account_status = AccountStatus.DEACTIVATED
1738 self.account_status_comment = comment1856 self.account_status_comment = comment
1739 IMasterObject(self.preferredemail).status = EmailAddressStatus.NEW1857 IMasterObject(self.preferredemail).status = EmailAddressStatus.NEW
1740 self._preferredemail_cached = None1858 clear_property(self, '_preferredemail_cached')
1741 base_new_name = self.name + '-deactivatedaccount'1859 base_new_name = self.name + '-deactivatedaccount'
1742 self.name = self._ensureNewName(base_new_name)1860 self.name = self._ensureNewName(base_new_name)
17431861
@@ -2070,7 +2188,7 @@
2070 if email_address is not None:2188 if email_address is not None:
2071 email_address.status = EmailAddressStatus.VALIDATED2189 email_address.status = EmailAddressStatus.VALIDATED
2072 email_address.syncUpdate()2190 email_address.syncUpdate()
2073 self._preferredemail_cached = None2191 clear_property(self, '_preferredemail_cached')
20742192
2075 def setPreferredEmail(self, email):2193 def setPreferredEmail(self, email):
2076 """See `IPerson`."""2194 """See `IPerson`."""
@@ -2107,7 +2225,7 @@
2107 IMasterObject(email).syncUpdate()2225 IMasterObject(email).syncUpdate()
21082226
2109 # Now we update our cache of the preferredemail.2227 # Now we update our cache of the preferredemail.
2110 self._preferredemail_cached = email2228 cache_property(self, '_preferredemail_cached', email)
21112229
2112 @cachedproperty('_preferredemail_cached')2230 @cachedproperty('_preferredemail_cached')
2113 def preferredemail(self):2231 def preferredemail(self):
@@ -2274,17 +2392,23 @@
2274 distribution.main_archive, self)2392 distribution.main_archive, self)
2275 return permissions.count() > 02393 return permissions.count() > 0
22762394
2277 @cachedproperty2395 @cachedproperty('_is_ubuntu_coc_signer_cached')
2278 def is_ubuntu_coc_signer(self):2396 def is_ubuntu_coc_signer(self):
2279 """See `IPerson`."""2397 """See `IPerson`."""
2398 # Also assigned to by self._all_members.
2399 store = Store.of(self)
2400 query = AND(SignedCodeOfConduct.ownerID == self.id,
2401 Person._is_ubuntu_coc_signer_condition())
2402 # TODO: Using exists would be faster than count().
2403 return bool(store.find(SignedCodeOfConduct, query).count())
2404
2405 @staticmethod
2406 def _is_ubuntu_coc_signer_condition():
2407 """Generate a Storm Expr for determing the coc signing status."""
2280 sigset = getUtility(ISignedCodeOfConductSet)2408 sigset = getUtility(ISignedCodeOfConductSet)
2281 lastdate = sigset.getLastAcceptedDate()2409 lastdate = sigset.getLastAcceptedDate()
22822410 return AND(SignedCodeOfConduct.active == True,
2283 query = AND(SignedCodeOfConduct.q.active==True,2411 SignedCodeOfConduct.datecreated >= lastdate)
2284 SignedCodeOfConduct.q.ownerID==self.id,
2285 SignedCodeOfConduct.q.datecreated>=lastdate)
2286
2287 return bool(SignedCodeOfConduct.select(query).count())
22882412
2289 @property2413 @property
2290 def activesignatures(self):2414 def activesignatures(self):
@@ -2298,7 +2422,7 @@
2298 sCoC_util = getUtility(ISignedCodeOfConductSet)2422 sCoC_util = getUtility(ISignedCodeOfConductSet)
2299 return sCoC_util.searchByUser(self.id, active=False)2423 return sCoC_util.searchByUser(self.id, active=False)
23002424
2301 @property2425 @cachedproperty('_archive_cached')
2302 def archive(self):2426 def archive(self):
2303 """See `IPerson`."""2427 """See `IPerson`."""
2304 return getUtility(IArchiveSet).getPPAOwnedByPerson(self)2428 return getUtility(IArchiveSet).getPPAOwnedByPerson(self)
@@ -2622,7 +2746,8 @@
2622 # Populate the previously empty 'preferredemail' cached2746 # Populate the previously empty 'preferredemail' cached
2623 # property, so the Person record is up-to-date.2747 # property, so the Person record is up-to-date.
2624 if master_email.status == EmailAddressStatus.PREFERRED:2748 if master_email.status == EmailAddressStatus.PREFERRED:
2625 account_person._preferredemail_cached = master_email2749 cache_property(account_person, '_preferredemail_cached',
2750 master_email)
2626 return account_person2751 return account_person
2627 # There is no associated `Person` to the email `Account`.2752 # There is no associated `Person` to the email `Account`.
2628 # This is probably because the account was created externally2753 # This is probably because the account was created externally
26292754
=== 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 @@
509509
510 def __storm_invalidated__(self):510 def __storm_invalidated__(self):
511 """Clear cached non-storm attributes when the transaction ends."""511 """Clear cached non-storm attributes when the transaction ends."""
512 super(Product, self).__storm_invalidated__()
512 self._cached_licenses = None513 self._cached_licenses = None
513 if safe_hasattr(self, '_commercial_subscription_cached'):
514 del self._commercial_subscription_cached
515514
516 def _getLicenses(self):515 def _getLicenses(self):
517 """Get the licenses as a tuple."""516 """Get the licenses as a tuple."""
518517
=== 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 @@
1# Copyright 2009-2010 Canonical Ltd. This software is licensed under the1# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4from __future__ import with_statement
5
4__metaclass__ = type6__metaclass__ = type
57
6from datetime import datetime8from datetime import datetime
7import pytz9import pytz
8import time10import time
911
12from testtools.matchers import LessThan
13
10import transaction14import transaction
1115
12from zope.component import getUtility16from zope.component import getUtility
@@ -26,6 +30,7 @@
26 IPersonSet, ImmutableVisibilityError, NameAlreadyTaken,30 IPersonSet, ImmutableVisibilityError, NameAlreadyTaken,
27 PersonCreationRationale, PersonVisibility)31 PersonCreationRationale, PersonVisibility)
28from canonical.launchpad.database import Bug32from canonical.launchpad.database import Bug
33from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
29from lp.registry.model.structuralsubscription import (34from lp.registry.model.structuralsubscription import (
30 StructuralSubscription)35 StructuralSubscription)
31from lp.registry.model.karma import KarmaCategory36from lp.registry.model.karma import KarmaCategory
@@ -34,8 +39,12 @@
34from lp.bugs.interfaces.bugtask import IllegalRelatedBugTasksParams39from lp.bugs.interfaces.bugtask import IllegalRelatedBugTasksParams
35from lp.answers.model.answercontact import AnswerContact40from lp.answers.model.answercontact import AnswerContact
36from lp.blueprints.model.specification import Specification41from lp.blueprints.model.specification import Specification
37from lp.testing import login_person, logout, TestCase, TestCaseWithFactory42from lp.testing import (
43 login_person, logout, person_logged_in, TestCase, TestCaseWithFactory,
44 )
45from lp.testing.matchers import HasQueryCount
38from lp.testing.views import create_initialized_view46from lp.testing.views import create_initialized_view
47from lp.testing._webservice import QueryCollector
39from lp.registry.interfaces.person import PrivatePersonLinkageError48from lp.registry.interfaces.person import PrivatePersonLinkageError
40from canonical.testing.layers import DatabaseFunctionalLayer, reconnect_stores49from canonical.testing.layers import DatabaseFunctionalLayer, reconnect_stores
4150
@@ -191,7 +200,8 @@
191200
192 def test_person_snapshot(self):201 def test_person_snapshot(self):
193 omitted = (202 omitted = (
194 'activemembers', 'adminmembers', 'allmembers', 'approvedmembers',203 'activemembers', 'adminmembers', 'allmembers',
204 'all_members_prepopulated', 'approvedmembers',
195 'deactivatedmembers', 'expiredmembers', 'inactivemembers',205 'deactivatedmembers', 'expiredmembers', 'inactivemembers',
196 'invited_members', 'member_memberships', 'pendingmembers',206 'invited_members', 'member_memberships', 'pendingmembers',
197 'proposedmembers', 'unmapped_participants',207 'proposedmembers', 'unmapped_participants',
@@ -553,3 +563,31 @@
553 names = [entry['project'].name for entry in results]563 names = [entry['project'].name for entry in results]
554 self.assertEqual(564 self.assertEqual(
555 ['cc', 'bb', 'aa', 'dd', 'ee'], names)565 ['cc', 'bb', 'aa', 'dd', 'ee'], names)
566
567
568class TestAPIPartipication(TestCaseWithFactory):
569
570 layer = DatabaseFunctionalLayer
571
572 def test_participation_query_limit(self):
573 # A team with 3 members should only query once for all their
574 # attributes.
575 team = self.factory.makeTeam()
576 with person_logged_in(team.teamowner):
577 team.addMember(self.factory.makePerson(), team.teamowner)
578 team.addMember(self.factory.makePerson(), team.teamowner)
579 team.addMember(self.factory.makePerson(), team.teamowner)
580 webservice = LaunchpadWebServiceCaller()
581 collector = QueryCollector()
582 collector.register()
583 self.addCleanup(collector.unregister)
584 url = "/~%s/participants" % team.name
585 logout()
586 response = webservice.get(url, headers={'User-Agent':'AnonNeedsThis'})
587 self.assertEqual(response.status, 200,
588 "Got %d for url %r with response %r" % (
589 response.status, url, response.body))
590 # XXX: This number should really be 10, but see
591 # https://bugs.launchpad.net/storm/+bug/619017 which is adding 3
592 # queries to the test.
593 self.assertThat(collector, HasQueryCount(LessThan(13)))
556594
=== 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 @@
25from lp.app.errors import NotFoundError25from lp.app.errors import NotFoundError
26from lp.archivepublisher.debversion import Version26from lp.archivepublisher.debversion import Version
27from lp.archiveuploader.utils import re_issource, re_isadeb27from lp.archiveuploader.utils import re_issource, re_isadeb
28from canonical.cachedproperty import clear_property
28from canonical.config import config29from canonical.config import config
29from canonical.database.constants import UTC_NOW30from canonical.database.constants import UTC_NOW
30from canonical.database.datetimecol import UtcDateTimeCol31from canonical.database.datetimecol import UtcDateTimeCol
@@ -1753,8 +1754,12 @@
17531754
1754 # Signing-key for the default PPA is reused when it's already present.1755 # Signing-key for the default PPA is reused when it's already present.
1755 signing_key = None1756 signing_key = None
1756 if purpose == ArchivePurpose.PPA and owner.archive is not None:1757 if purpose == ArchivePurpose.PPA:
1757 signing_key = owner.archive.signing_key1758 if owner.archive is not None:
1759 signing_key = owner.archive.signing_key
1760 else:
1761 # owner.archive is a cached property and we've just cached it.
1762 clear_property(owner, '_archive_cached')
17581763
1759 new_archive = Archive(1764 new_archive = Archive(
1760 owner=owner, distribution=distribution, name=name,1765 owner=owner, distribution=distribution, name=name,
@@ -1806,6 +1811,7 @@
18061811
1807 def getPPAOwnedByPerson(self, person, name=None):1812 def getPPAOwnedByPerson(self, person, name=None):
1808 """See `IArchiveSet`."""1813 """See `IArchiveSet`."""
1814 # See Person._all_members which also directly queries this.
1809 store = Store.of(person)1815 store = Store.of(person)
1810 clause = [1816 clause = [
1811 Archive.purpose == ArchivePurpose.PPA,1817 Archive.purpose == ArchivePurpose.PPA,
18121818
=== 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 @@
185 _uses_english_msgids = None185 _uses_english_msgids = None
186186
187 def __storm_invalidated__(self):187 def __storm_invalidated__(self):
188 super(POTemplate, self).__storm_invalidated__()
188 self.clearPOFileCache()189 self.clearPOFileCache()
189 self._uses_english_msgids = None190 self._uses_english_msgids = None
190191
191192
=== 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 @@
103 credits_message_ids = credits_message_info.keys()103 credits_message_ids = credits_message_info.keys()
104104
105 def __storm_invalidated__(self):105 def __storm_invalidated__(self):
106 super(POTMsgSet, self).__storm_invalidated__()
106 self._cached_singular_text = None107 self._cached_singular_text = None
107 self._cached_uses_english_msgids = None108 self._cached_uses_english_msgids = None
108109
@@ -157,6 +158,7 @@
157 @property158 @property
158 def uses_english_msgids(self):159 def uses_english_msgids(self):
159 """See `IPOTMsgSet`."""160 """See `IPOTMsgSet`."""
161 # TODO: convert to cachedproperty, it will be simpler.
160 if self._cached_uses_english_msgids is not None:162 if self._cached_uses_english_msgids is not None:
161 return self._cached_uses_english_msgids163 return self._cached_uses_english_msgids
162164
@@ -181,6 +183,7 @@
181 @property183 @property
182 def singular_text(self):184 def singular_text(self):
183 """See `IPOTMsgSet`."""185 """See `IPOTMsgSet`."""
186 # TODO: convert to cachedproperty, it will be simpler.
184 if self._cached_singular_text is not None:187 if self._cached_singular_text is not None:
185 return self._cached_singular_text188 return self._cached_singular_text
186189