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
1=== modified file 'lib/canonical/cachedproperty.py'
2--- lib/canonical/cachedproperty.py 2010-01-11 21:29:33 +0000
3+++ lib/canonical/cachedproperty.py 2010-08-17 02:31:51 +0000
4@@ -3,10 +3,24 @@
5
6 """Cached properties for situations where a property is computed once and
7 then returned each time it is asked for.
8+
9+The clear_cachedproperties function can be used to wipe the cache of properties
10+from an instance.
11 """
12
13 __metaclass__ = type
14
15+__all__ = [
16+ 'cache_property',
17+ 'cachedproperty',
18+ 'clear_cachedproperties',
19+ 'clear_property',
20+ ]
21+
22+from zope.security.proxy import removeSecurityProxy
23+
24+from canonical.lazr.utils import safe_hasattr
25+
26 # XXX: JonathanLange 2010-01-11 bug=505731: Move this to lp.services.
27
28 def cachedproperty(attrname_or_fn):
29@@ -17,6 +31,10 @@
30
31 If you don't provide a name, the mangled name of the property is used.
32
33+ cachedproperty is not threadsafe - it should not be used on objects which
34+ are shared across threads / external locking should be used on those
35+ objects.
36+
37 >>> class CachedPropertyTest(object):
38 ...
39 ... @cachedproperty('_foo_cache')
40@@ -44,6 +62,10 @@
41 69
42 >>> cpt._bar_cached_value
43 69
44+
45+ Cached properties are listed on instances.
46+ >>> sorted(cpt._cached_properties)
47+ ['_bar_cached_value', '_foo_cache']
48
49 """
50 if isinstance(attrname_or_fn, basestring):
51@@ -54,8 +76,125 @@
52 attrname = '_%s_cached_value' % fn.__name__
53 return CachedProperty(attrname, fn)
54
55+def cache_property(instance, attrname, value):
56+ """Cache value on instance as attrname.
57+
58+ instance._cached_properties is updated with attrname.
59+
60+ >>> class CachedPropertyTest(object):
61+ ...
62+ ... @cachedproperty('_foo_cache')
63+ ... def foo(self):
64+ ... return 23
65+ ...
66+ >>> instance = CachedPropertyTest()
67+ >>> cache_property(instance, '_foo_cache', 42)
68+ >>> instance.foo
69+ 42
70+ >>> instance._cached_properties
71+ ['_foo_cache']
72+
73+ Caching a new value does not duplicate the cache keys.
74+
75+ >>> cache_property(instance, '_foo_cache', 84)
76+ >>> instance._cached_properties
77+ ['_foo_cache']
78+
79+ And does update the cached value.
80+
81+ >>> instance.foo
82+ 84
83+ """
84+ naked_instance = removeSecurityProxy(instance)
85+ clear_property(naked_instance, attrname)
86+ setattr(naked_instance, attrname, value)
87+ cached_properties = getattr(naked_instance, '_cached_properties', [])
88+ cached_properties.append(attrname)
89+ naked_instance._cached_properties = cached_properties
90+
91+
92+def clear_property(instance, attrname):
93+ """Remove a cached attribute from instance.
94+
95+ The attribute name is removed from instance._cached_properties.
96+
97+ If the property is not cached, nothing happens.
98+
99+ :seealso clear_cachedproperties: For clearing all cached items at once.
100+
101+ >>> class CachedPropertyTest(object):
102+ ...
103+ ... @cachedproperty('_foo_cache')
104+ ... def foo(self):
105+ ... return 23
106+ ...
107+ >>> instance = CachedPropertyTest()
108+ >>> instance.foo
109+ 23
110+ >>> clear_property(instance, '_foo_cache')
111+ >>> instance._cached_properties
112+ []
113+ >>> is_cached(instance, '_foo_cache')
114+ False
115+ >>> clear_property(instance, '_foo_cache')
116+ """
117+ naked_instance = removeSecurityProxy(instance)
118+ if not is_cached(naked_instance, attrname):
119+ return
120+ delattr(naked_instance, attrname)
121+ naked_instance._cached_properties.remove(attrname)
122+
123+
124+def clear_cachedproperties(instance):
125+ """Clear cached properties from an object.
126+
127+ >>> class CachedPropertyTest(object):
128+ ...
129+ ... @cachedproperty('_foo_cache')
130+ ... def foo(self):
131+ ... return 23
132+ ...
133+ >>> instance = CachedPropertyTest()
134+ >>> instance.foo
135+ 23
136+ >>> instance._cached_properties
137+ ['_foo_cache']
138+ >>> clear_cachedproperties(instance)
139+ >>> instance._cached_properties
140+ []
141+ >>> hasattr(instance, '_foo_cache')
142+ False
143+ """
144+ naked_instance = removeSecurityProxy(instance)
145+ cached_properties = getattr(naked_instance, '_cached_properties', [])
146+ for property_name in cached_properties:
147+ delattr(naked_instance, property_name)
148+ naked_instance._cached_properties = []
149+
150+
151+def is_cached(instance, attrname):
152+ """Return True if attrname is cached on instance.
153+
154+ >>> class CachedPropertyTest(object):
155+ ...
156+ ... @cachedproperty('_foo_cache')
157+ ... def foo(self):
158+ ... return 23
159+ ...
160+ >>> instance = CachedPropertyTest()
161+ >>> instance.foo
162+ 23
163+ >>> is_cached(instance, '_foo_cache')
164+ True
165+ >>> is_cached(instance, '_var_cache')
166+ False
167+ """
168+ naked_instance = removeSecurityProxy(instance)
169+ return safe_hasattr(naked_instance, attrname)
170+
171
172 class CachedPropertyForAttr:
173+ """Curry a decorator to provide arguments to the CachedProperty."""
174
175 def __init__(self, attrname):
176 self.attrname = attrname
177@@ -66,18 +205,20 @@
178
179 class CachedProperty:
180
181+ # Used to detect not-yet-cached properties.
182+ sentinel = object()
183+
184 def __init__(self, attrname, fn):
185 self.fn = fn
186 self.attrname = attrname
187- self.marker = object()
188
189 def __get__(self, inst, cls=None):
190 if inst is None:
191 return self
192- cachedresult = getattr(inst, self.attrname, self.marker)
193- if cachedresult is self.marker:
194+ cachedresult = getattr(inst, self.attrname, CachedProperty.sentinel)
195+ if cachedresult is CachedProperty.sentinel:
196 result = self.fn(inst)
197- setattr(inst, self.attrname, result)
198+ cache_property(inst, self.attrname, result)
199 return result
200 else:
201 return cachedresult
202
203=== modified file 'lib/canonical/database/sqlbase.py'
204--- lib/canonical/database/sqlbase.py 2010-05-21 13:08:18 +0000
205+++ lib/canonical/database/sqlbase.py 2010-08-17 02:31:51 +0000
206@@ -30,6 +30,7 @@
207
208 from lazr.restful.interfaces import IRepresentationCache
209
210+from canonical.cachedproperty import clear_cachedproperties
211 from canonical.config import config, dbconfig
212 from canonical.database.interfaces import ISQLBase
213
214@@ -175,6 +176,9 @@
215 correct master Store.
216 """
217 from canonical.launchpad.interfaces import IMasterStore
218+ # Make it simple to write dumb-invalidators - initialised
219+ # _cached_properties to a valid list rather than just-in-time creation.
220+ self._cached_properties = []
221 store = IMasterStore(self.__class__)
222
223 # The constructor will fail if objects from a different Store
224@@ -253,6 +257,14 @@
225 cache = getUtility(IRepresentationCache)
226 cache.delete(self)
227
228+ def __storm_invalidated__(self):
229+ """Flush cached properties."""
230+ # Note this is not directly tested; but the entire test suite blows up
231+ # awesomely if its broken : its entirely unclear where tests for this
232+ # should be -- RBC 20100816.
233+ clear_cachedproperties(self)
234+
235+
236 alreadyInstalledMsg = ("A ZopelessTransactionManager with these settings is "
237 "already installed. This is probably caused by calling initZopeless twice.")
238
239
240=== modified file 'lib/canonical/launchpad/database/librarian.py'
241--- lib/canonical/launchpad/database/librarian.py 2010-07-06 16:12:46 +0000
242+++ lib/canonical/launchpad/database/librarian.py 2010-08-17 02:31:51 +0000
243@@ -194,6 +194,7 @@
244
245 def __storm_invalidated__(self):
246 """Make sure that the file is closed across transaction boundary."""
247+ super(LibraryFileAlias, self).__storm_invalidated__()
248 self.close()
249
250
251
252=== modified file 'lib/lp/registry/doc/teammembership.txt'
253--- lib/lp/registry/doc/teammembership.txt 2010-07-14 15:28:42 +0000
254+++ lib/lp/registry/doc/teammembership.txt 2010-08-17 02:31:51 +0000
255@@ -1007,7 +1007,8 @@
256 >>> from canonical.launchpad.interfaces import IMasterObject
257 >>> IMasterObject(bad_user.account).status = AccountStatus.SUSPENDED
258 >>> IMasterObject(bad_user.preferredemail).status = EmailAddressStatus.OLD
259- >>> removeSecurityProxy(bad_user)._preferredemail_cached = None
260+ >>> from canonical.cachedproperty import clear_property
261+ >>> clear_property(removeSecurityProxy(bad_user), '_preferredemail_cached')
262 >>> transaction.commit()
263
264 >>> [m.displayname for m in t3.allmembers]
265
266=== modified file 'lib/lp/registry/interfaces/person.py'
267--- lib/lp/registry/interfaces/person.py 2010-08-03 08:49:19 +0000
268+++ lib/lp/registry/interfaces/person.py 2010-08-17 02:31:51 +0000
269@@ -1231,7 +1231,7 @@
270 all_member_count = Attribute(
271 "The total number of real people who are members of this team, "
272 "including subteams.")
273- allmembers = exported(
274+ all_members_prepopulated = exported(
275 doNotSnapshot(
276 CollectionField(
277 title=_("All participants of this team."),
278@@ -1243,6 +1243,8 @@
279 "IPerson.inTeam()."),
280 value_type=Reference(schema=Interface))),
281 exported_as='participants')
282+ allmembers = doNotSnapshot(
283+ Attribute("List of all members, without checking karma etc."))
284 approvedmembers = doNotSnapshot(
285 Attribute("List of members with APPROVED status"))
286 deactivated_member_count = Attribute("Number of deactivated members")
287@@ -1322,14 +1324,17 @@
288 center_lat, and center_lng
289 """
290
291- def getMembersWithPreferredEmails(include_teams=False):
292+ def getMembersWithPreferredEmails():
293 """Returns a result set of persons with precached addresses.
294
295 Persons or teams without preferred email addresses are not included.
296 """
297
298- def getMembersWithPreferredEmailsCount(include_teams=False):
299- """Returns the count of persons/teams with preferred emails."""
300+ def getMembersWithPreferredEmailsCount():
301+ """Returns the count of persons/teams with preferred emails.
302+
303+ See also getMembersWithPreferredEmails.
304+ """
305
306 def getDirectAdministrators():
307 """Return this team's administrators.
308@@ -2123,9 +2128,16 @@
309
310
311 # Fix value_type.schema of IPersonViewRestricted attributes.
312-for name in ['allmembers', 'activemembers', 'adminmembers', 'proposedmembers',
313- 'invited_members', 'deactivatedmembers', 'expiredmembers',
314- 'unmapped_participants']:
315+for name in [
316+ 'all_members_prepopulated',
317+ 'activemembers',
318+ 'adminmembers',
319+ 'proposedmembers',
320+ 'invited_members',
321+ 'deactivatedmembers',
322+ 'expiredmembers',
323+ 'unmapped_participants',
324+ ]:
325 IPersonViewRestricted[name].value_type.schema = IPerson
326
327 IPersonPublic['sub_teams'].value_type.schema = ITeam
328
329=== modified file 'lib/lp/registry/model/distribution.py'
330--- lib/lp/registry/model/distribution.py 2010-08-11 02:17:14 +0000
331+++ lib/lp/registry/model/distribution.py 2010-08-17 02:31:51 +0000
332@@ -19,7 +19,7 @@
333 from zope.interface import alsoProvides, implements
334
335 from lp.archivepublisher.debversion import Version
336-from canonical.cachedproperty import cachedproperty
337+from canonical.cachedproperty import cachedproperty, clear_property
338 from canonical.database.constants import UTC_NOW
339
340 from canonical.database.datetimecol import UtcDateTimeCol
341@@ -1617,8 +1617,9 @@
342 # This driver is a release manager.
343 series.driver = owner
344
345- if safe_hasattr(self, '_cached_series'):
346- del self._cached_series
347+ # May wish to add this to the series rather than clearing the cache --
348+ # RBC 20100816.
349+ clear_property(self, '_cached_series')
350 return series
351
352 @property
353
354=== modified file 'lib/lp/registry/model/person.py'
355--- lib/lp/registry/model/person.py 2010-08-13 08:26:15 +0000
356+++ lib/lp/registry/model/person.py 2010-08-17 02:31:51 +0000
357@@ -48,7 +48,9 @@
358 StringCol)
359 from sqlobject.sqlbuilder import AND, OR, SQLConstant
360 from storm.store import EmptyResultSet, Store
361-from storm.expr import And, In, Join, LeftJoin, Lower, Not, Or, SQL
362+from storm.expr import (
363+ Alias, And, Exists, In, Join, LeftJoin, Lower, Min, Not, Or, Select, SQL,
364+ )
365 from storm.info import ClassAlias
366
367 from canonical.config import config
368@@ -59,10 +61,11 @@
369 from canonical.database.sqlbase import (
370 cursor, quote, quote_like, sqlvalues, SQLBase)
371
372-from canonical.cachedproperty import cachedproperty
373+from canonical.cachedproperty import cachedproperty, cache_property, clear_property
374
375 from canonical.lazr.utils import get_current_browser_request, safe_hasattr
376
377+from canonical.launchpad.components.decoratedresultset import DecoratedResultSet
378 from canonical.launchpad.database.account import Account, AccountPassword
379 from canonical.launchpad.interfaces.account import AccountSuspendedError
380 from lp.bugs.model.bugtarget import HasBugsBase
381@@ -390,13 +393,12 @@
382
383 Order them by name if necessary.
384 """
385- self._languages_cache = sorted(
386- languages, key=attrgetter('englishname'))
387+ cache_property(self, '_languages_cache', sorted(
388+ languages, key=attrgetter('englishname')))
389
390 def deleteLanguagesCache(self):
391 """Delete this person's cached languages, if it exists."""
392- if safe_hasattr(self, '_languages_cache'):
393- del self._languages_cache
394+ clear_property(self, '_languages_cache')
395
396 def addLanguage(self, language):
397 """See `IPerson`."""
398@@ -1012,9 +1014,10 @@
399 result = result.order_by(KarmaCategory.title)
400 return [karma_cache for (karma_cache, category) in result]
401
402- @property
403+ @cachedproperty('_karma_cached')
404 def karma(self):
405 """See `IPerson`."""
406+ # May also be loaded from _all_members
407 cache = KarmaTotalCache.selectOneBy(person=self)
408 if cache is None:
409 # Newly created accounts may not be in the cache yet, meaning the
410@@ -1032,7 +1035,7 @@
411
412 return self.is_valid_person
413
414- @property
415+ @cachedproperty('_is_valid_person_cached')
416 def is_valid_person(self):
417 """See `IPerson`."""
418 if self.is_team:
419@@ -1431,14 +1434,131 @@
420 @property
421 def allmembers(self):
422 """See `IPerson`."""
423- query = """
424- Person.id = TeamParticipation.person AND
425- TeamParticipation.team = %s AND
426- TeamParticipation.person != %s
427- """ % sqlvalues(self.id, self.id)
428- return Person.select(query, clauseTables=['TeamParticipation'])
429-
430- def _getMembersWithPreferredEmails(self, include_teams=False):
431+ return self._all_members()
432+
433+ @property
434+ def all_members_prepopulated(self):
435+ """See `IPerson`."""
436+ return self._all_members(need_karma=True, need_ubuntu_coc=True,
437+ need_location=True, need_archive=True, need_preferred_email=True,
438+ need_validity=True)
439+
440+ def _all_members(self, need_karma=False, need_ubuntu_coc=False,
441+ need_location=False, need_archive=False, need_preferred_email=False,
442+ need_validity=False):
443+ """Lookup all members of the team with optional precaching.
444+
445+ :param need_karma: The karma attribute will be cached.
446+ :param need_ubuntu_coc: The is_ubuntu_coc_signer attribute will be
447+ cached.
448+ :param need_location: The location attribute will be cached.
449+ :param need_archive: The archive attribute will be cached.
450+ :param need_preferred_email: The preferred email attribute will be
451+ cached.
452+ :param need_validity: The is_valid attribute will be cached.
453+ """
454+ # TODO: consolidate this with getMembersWithPreferredEmails.
455+ # The difference between the two is that
456+ # getMembersWithPreferredEmails includes self, which is arguably
457+ # wrong, but perhaps deliberate.
458+ store = Store.of(self)
459+ origin = [
460+ Person,
461+ Join(TeamParticipation, TeamParticipation.person == Person.id),
462+ ]
463+ conditions = And(
464+ # Members of this team,
465+ TeamParticipation.team == self.id,
466+ # But not the team itself.
467+ TeamParticipation.person != self.id)
468+ columns = [Person]
469+ if need_karma:
470+ # New people have no karmatotalcache rows.
471+ origin.append(
472+ LeftJoin(KarmaTotalCache, KarmaTotalCache.person == Person.id))
473+ columns.append(KarmaTotalCache)
474+ if need_ubuntu_coc:
475+ columns.append(Alias(Exists(Select(SignedCodeOfConduct,
476+ AND(Person._is_ubuntu_coc_signer_condition(),
477+ SignedCodeOfConduct.ownerID == Person.id))),
478+ name='is_ubuntu_coc_signer'))
479+ if need_location:
480+ # New people have no location rows
481+ origin.append(
482+ LeftJoin(PersonLocation, PersonLocation.person == Person.id))
483+ columns.append(PersonLocation)
484+ if need_archive:
485+ # Not everyone has PPA's
486+ # It would be nice to cleanly expose the soyuz rules for this to avoid
487+ # duplicating the relationships.
488+ origin.append(
489+ LeftJoin(Archive, Archive.owner == Person.id))
490+ columns.append(Archive)
491+ conditions = And(conditions,
492+ Or(Archive.id == None, And(
493+ Archive.id == Select(Min(Archive.id),
494+ Archive.owner == Person.id, Archive),
495+ Archive.purpose == ArchivePurpose.PPA)))
496+ if need_preferred_email:
497+ # Teams don't have email.
498+ origin.append(
499+ LeftJoin(EmailAddress, EmailAddress.person == Person.id))
500+ columns.append(EmailAddress)
501+ conditions = And(conditions,
502+ Or(EmailAddress.status == None,
503+ EmailAddress.status == EmailAddressStatus.PREFERRED))
504+ if need_validity:
505+ # May find invalid persons
506+ origin.append(
507+ LeftJoin(ValidPersonCache, ValidPersonCache.id == Person.id))
508+ columns.append(ValidPersonCache)
509+ if len(columns) == 1:
510+ columns = columns[0]
511+ # Return a simple ResultSet
512+ return store.using(*origin).find(columns, conditions)
513+ # Adapt the result into a cached Person.
514+ columns = tuple(columns)
515+ raw_result = store.using(*origin).find(columns, conditions)
516+ def prepopulate_person(row):
517+ result = row[0]
518+ index = 1
519+ #-- karma caching
520+ if need_karma:
521+ karma = row[index]
522+ index += 1
523+ if karma is None:
524+ karma_total = 0
525+ else:
526+ karma_total = karma.karma_total
527+ cache_property(result, '_karma_cached', karma_total)
528+ #-- ubuntu code of conduct signer status caching.
529+ if need_ubuntu_coc:
530+ signed = row[index]
531+ index += 1
532+ cache_property(result, '_is_ubuntu_coc_signer_cached', signed)
533+ #-- location caching
534+ if need_location:
535+ location = row[index]
536+ index += 1
537+ cache_property(result, '_location', location)
538+ #-- archive caching
539+ if need_archive:
540+ archive = row[index]
541+ index += 1
542+ cache_property(result, '_archive_cached', archive)
543+ #-- preferred email caching
544+ if need_preferred_email:
545+ email = row[index]
546+ index += 1
547+ cache_property(result, '_preferredemail_cached', email)
548+ if need_validity:
549+ valid = row[index] is not None
550+ index += 1
551+ cache_property(result, '_is_valid_person_cached', valid)
552+ return result
553+ return DecoratedResultSet(raw_result, result_decorator=prepopulate_person)
554+
555+ def _getMembersWithPreferredEmails(self):
556 """Helper method for public getMembersWithPreferredEmails.
557
558 We can't return the preferred email address directly to the
559@@ -1456,20 +1576,18 @@
560 EmailAddress.status == EmailAddressStatus.PREFERRED)
561 return store.using(*origin).find((Person, EmailAddress), conditions)
562
563- def getMembersWithPreferredEmails(self, include_teams=False):
564+ def getMembersWithPreferredEmails(self):
565 """See `IPerson`."""
566- result = self._getMembersWithPreferredEmails(
567- include_teams=include_teams)
568+ result = self._getMembersWithPreferredEmails()
569 person_list = []
570 for person, email in result:
571- person._preferredemail_cached = email
572+ cache_property(person, '_preferredemail_cached', email)
573 person_list.append(person)
574 return person_list
575
576- def getMembersWithPreferredEmailsCount(self, include_teams=False):
577+ def getMembersWithPreferredEmailsCount(self):
578 """See `IPerson`."""
579- result = self._getMembersWithPreferredEmails(
580- include_teams=include_teams)
581+ result = self._getMembersWithPreferredEmails()
582 return result.count()
583
584 @property
585@@ -1737,7 +1855,7 @@
586 self.account_status = AccountStatus.DEACTIVATED
587 self.account_status_comment = comment
588 IMasterObject(self.preferredemail).status = EmailAddressStatus.NEW
589- self._preferredemail_cached = None
590+ clear_property(self, '_preferredemail_cached')
591 base_new_name = self.name + '-deactivatedaccount'
592 self.name = self._ensureNewName(base_new_name)
593
594@@ -2070,7 +2188,7 @@
595 if email_address is not None:
596 email_address.status = EmailAddressStatus.VALIDATED
597 email_address.syncUpdate()
598- self._preferredemail_cached = None
599+ clear_property(self, '_preferredemail_cached')
600
601 def setPreferredEmail(self, email):
602 """See `IPerson`."""
603@@ -2107,7 +2225,7 @@
604 IMasterObject(email).syncUpdate()
605
606 # Now we update our cache of the preferredemail.
607- self._preferredemail_cached = email
608+ cache_property(self, '_preferredemail_cached', email)
609
610 @cachedproperty('_preferredemail_cached')
611 def preferredemail(self):
612@@ -2274,17 +2392,23 @@
613 distribution.main_archive, self)
614 return permissions.count() > 0
615
616- @cachedproperty
617+ @cachedproperty('_is_ubuntu_coc_signer_cached')
618 def is_ubuntu_coc_signer(self):
619 """See `IPerson`."""
620+ # Also assigned to by self._all_members.
621+ store = Store.of(self)
622+ query = AND(SignedCodeOfConduct.ownerID == self.id,
623+ Person._is_ubuntu_coc_signer_condition())
624+ # TODO: Using exists would be faster than count().
625+ return bool(store.find(SignedCodeOfConduct, query).count())
626+
627+ @staticmethod
628+ def _is_ubuntu_coc_signer_condition():
629+ """Generate a Storm Expr for determing the coc signing status."""
630 sigset = getUtility(ISignedCodeOfConductSet)
631 lastdate = sigset.getLastAcceptedDate()
632-
633- query = AND(SignedCodeOfConduct.q.active==True,
634- SignedCodeOfConduct.q.ownerID==self.id,
635- SignedCodeOfConduct.q.datecreated>=lastdate)
636-
637- return bool(SignedCodeOfConduct.select(query).count())
638+ return AND(SignedCodeOfConduct.active == True,
639+ SignedCodeOfConduct.datecreated >= lastdate)
640
641 @property
642 def activesignatures(self):
643@@ -2298,7 +2422,7 @@
644 sCoC_util = getUtility(ISignedCodeOfConductSet)
645 return sCoC_util.searchByUser(self.id, active=False)
646
647- @property
648+ @cachedproperty('_archive_cached')
649 def archive(self):
650 """See `IPerson`."""
651 return getUtility(IArchiveSet).getPPAOwnedByPerson(self)
652@@ -2622,7 +2746,8 @@
653 # Populate the previously empty 'preferredemail' cached
654 # property, so the Person record is up-to-date.
655 if master_email.status == EmailAddressStatus.PREFERRED:
656- account_person._preferredemail_cached = master_email
657+ cache_property(account_person, '_preferredemail_cached',
658+ master_email)
659 return account_person
660 # There is no associated `Person` to the email `Account`.
661 # This is probably because the account was created externally
662
663=== modified file 'lib/lp/registry/model/product.py'
664--- lib/lp/registry/model/product.py 2010-08-06 14:49:35 +0000
665+++ lib/lp/registry/model/product.py 2010-08-17 02:31:51 +0000
666@@ -509,9 +509,8 @@
667
668 def __storm_invalidated__(self):
669 """Clear cached non-storm attributes when the transaction ends."""
670+ super(Product, self).__storm_invalidated__()
671 self._cached_licenses = None
672- if safe_hasattr(self, '_commercial_subscription_cached'):
673- del self._commercial_subscription_cached
674
675 def _getLicenses(self):
676 """Get the licenses as a tuple."""
677
678=== modified file 'lib/lp/registry/tests/test_person.py'
679--- lib/lp/registry/tests/test_person.py 2010-08-13 07:48:54 +0000
680+++ lib/lp/registry/tests/test_person.py 2010-08-17 02:31:51 +0000
681@@ -1,12 +1,16 @@
682 # Copyright 2009-2010 Canonical Ltd. This software is licensed under the
683 # GNU Affero General Public License version 3 (see the file LICENSE).
684
685+from __future__ import with_statement
686+
687 __metaclass__ = type
688
689 from datetime import datetime
690 import pytz
691 import time
692
693+from testtools.matchers import LessThan
694+
695 import transaction
696
697 from zope.component import getUtility
698@@ -26,6 +30,7 @@
699 IPersonSet, ImmutableVisibilityError, NameAlreadyTaken,
700 PersonCreationRationale, PersonVisibility)
701 from canonical.launchpad.database import Bug
702+from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
703 from lp.registry.model.structuralsubscription import (
704 StructuralSubscription)
705 from lp.registry.model.karma import KarmaCategory
706@@ -34,8 +39,12 @@
707 from lp.bugs.interfaces.bugtask import IllegalRelatedBugTasksParams
708 from lp.answers.model.answercontact import AnswerContact
709 from lp.blueprints.model.specification import Specification
710-from lp.testing import login_person, logout, TestCase, TestCaseWithFactory
711+from lp.testing import (
712+ login_person, logout, person_logged_in, TestCase, TestCaseWithFactory,
713+ )
714+from lp.testing.matchers import HasQueryCount
715 from lp.testing.views import create_initialized_view
716+from lp.testing._webservice import QueryCollector
717 from lp.registry.interfaces.person import PrivatePersonLinkageError
718 from canonical.testing.layers import DatabaseFunctionalLayer, reconnect_stores
719
720@@ -191,7 +200,8 @@
721
722 def test_person_snapshot(self):
723 omitted = (
724- 'activemembers', 'adminmembers', 'allmembers', 'approvedmembers',
725+ 'activemembers', 'adminmembers', 'allmembers',
726+ 'all_members_prepopulated', 'approvedmembers',
727 'deactivatedmembers', 'expiredmembers', 'inactivemembers',
728 'invited_members', 'member_memberships', 'pendingmembers',
729 'proposedmembers', 'unmapped_participants',
730@@ -553,3 +563,31 @@
731 names = [entry['project'].name for entry in results]
732 self.assertEqual(
733 ['cc', 'bb', 'aa', 'dd', 'ee'], names)
734+
735+
736+class TestAPIPartipication(TestCaseWithFactory):
737+
738+ layer = DatabaseFunctionalLayer
739+
740+ def test_participation_query_limit(self):
741+ # A team with 3 members should only query once for all their
742+ # attributes.
743+ team = self.factory.makeTeam()
744+ with person_logged_in(team.teamowner):
745+ team.addMember(self.factory.makePerson(), team.teamowner)
746+ team.addMember(self.factory.makePerson(), team.teamowner)
747+ team.addMember(self.factory.makePerson(), team.teamowner)
748+ webservice = LaunchpadWebServiceCaller()
749+ collector = QueryCollector()
750+ collector.register()
751+ self.addCleanup(collector.unregister)
752+ url = "/~%s/participants" % team.name
753+ logout()
754+ response = webservice.get(url, headers={'User-Agent':'AnonNeedsThis'})
755+ self.assertEqual(response.status, 200,
756+ "Got %d for url %r with response %r" % (
757+ response.status, url, response.body))
758+ # XXX: This number should really be 10, but see
759+ # https://bugs.launchpad.net/storm/+bug/619017 which is adding 3
760+ # queries to the test.
761+ self.assertThat(collector, HasQueryCount(LessThan(13)))
762
763=== modified file 'lib/lp/soyuz/model/archive.py'
764--- lib/lp/soyuz/model/archive.py 2010-08-05 13:23:52 +0000
765+++ lib/lp/soyuz/model/archive.py 2010-08-17 02:31:51 +0000
766@@ -25,6 +25,7 @@
767 from lp.app.errors import NotFoundError
768 from lp.archivepublisher.debversion import Version
769 from lp.archiveuploader.utils import re_issource, re_isadeb
770+from canonical.cachedproperty import clear_property
771 from canonical.config import config
772 from canonical.database.constants import UTC_NOW
773 from canonical.database.datetimecol import UtcDateTimeCol
774@@ -1753,8 +1754,12 @@
775
776 # Signing-key for the default PPA is reused when it's already present.
777 signing_key = None
778- if purpose == ArchivePurpose.PPA and owner.archive is not None:
779- signing_key = owner.archive.signing_key
780+ if purpose == ArchivePurpose.PPA:
781+ if owner.archive is not None:
782+ signing_key = owner.archive.signing_key
783+ else:
784+ # owner.archive is a cached property and we've just cached it.
785+ clear_property(owner, '_archive_cached')
786
787 new_archive = Archive(
788 owner=owner, distribution=distribution, name=name,
789@@ -1806,6 +1811,7 @@
790
791 def getPPAOwnedByPerson(self, person, name=None):
792 """See `IArchiveSet`."""
793+ # See Person._all_members which also directly queries this.
794 store = Store.of(person)
795 clause = [
796 Archive.purpose == ArchivePurpose.PPA,
797
798=== modified file 'lib/lp/translations/model/potemplate.py'
799--- lib/lp/translations/model/potemplate.py 2010-08-06 09:32:02 +0000
800+++ lib/lp/translations/model/potemplate.py 2010-08-17 02:31:51 +0000
801@@ -185,6 +185,7 @@
802 _uses_english_msgids = None
803
804 def __storm_invalidated__(self):
805+ super(POTemplate, self).__storm_invalidated__()
806 self.clearPOFileCache()
807 self._uses_english_msgids = None
808
809
810=== modified file 'lib/lp/translations/model/potmsgset.py'
811--- lib/lp/translations/model/potmsgset.py 2010-08-06 06:51:37 +0000
812+++ lib/lp/translations/model/potmsgset.py 2010-08-17 02:31:51 +0000
813@@ -103,6 +103,7 @@
814 credits_message_ids = credits_message_info.keys()
815
816 def __storm_invalidated__(self):
817+ super(POTMsgSet, self).__storm_invalidated__()
818 self._cached_singular_text = None
819 self._cached_uses_english_msgids = None
820
821@@ -157,6 +158,7 @@
822 @property
823 def uses_english_msgids(self):
824 """See `IPOTMsgSet`."""
825+ # TODO: convert to cachedproperty, it will be simpler.
826 if self._cached_uses_english_msgids is not None:
827 return self._cached_uses_english_msgids
828
829@@ -181,6 +183,7 @@
830 @property
831 def singular_text(self):
832 """See `IPOTMsgSet`."""
833+ # TODO: convert to cachedproperty, it will be simpler.
834 if self._cached_singular_text is not None:
835 return self._cached_singular_text
836