launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1393549 into lp:launchpad.

Commit message:
Preload API attributes when findTeam is invoked from the API. And preload teamowner when preloading Persons for the API.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1393549 in Launchpad itself: "Timeout when calling findTeam over the API"
  https://bugs.launchpad.net/launchpad/+bug/1393549

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1393549/+merge/242033

Preload API attributes when findTeam is invoked from the API. And preload teamowner when preloading Persons for the API.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-1393549/+merge/242033
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1393549 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py	2014-10-22 17:20:27 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py	2014-11-17 23:51:02 +0000
@@ -153,6 +153,23 @@
         self.assertReturnsPeople(
             [team_name], '/people?ws.op=findTeam&text=%s' % person_name)
 
+    def test_findTeam_query_count(self):
+        with admin_logged_in():
+            ws = webservice_for_person(self.factory.makePerson())
+
+        def create_match():
+            with admin_logged_in():
+                self.factory.makeTeam(displayname='foobar')
+
+        def find_teams():
+            ws.named_get('/people', 'findTeam', text="foobar").jsonBody()
+
+        # Ensure that we're already in a stable cache state.
+        find_teams()
+        recorder1, recorder2 = record_two_runs(
+            find_teams, create_match, 2)
+        self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
+
     def test_findPerson(self):
         # The search can be restricted to people.
         with admin_logged_in():

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2014-10-22 16:20:30 +0000
+++ lib/lp/registry/interfaces/person.py	2014-11-17 23:51:02 +0000
@@ -2269,12 +2269,13 @@
         restrict the search to the dates provided.
         """
 
+    @call_with(preload_for_api=True)
     @operation_parameters(
         text=TextLine(title=_("Search text"), default=u""))
     @operation_returns_collection_of(IPerson)
     @export_read_operation()
     @operation_for_version("beta")
-    def findTeam(text=""):
+    def findTeam(text="", preload_for_api=False):
         """Return all Teams whose name, displayname or email address
         match <text>.
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2014-10-22 19:10:06 +0000
+++ lib/lp/registry/model/person.py	2014-11-17 23:51:02 +0000
@@ -3658,7 +3658,7 @@
         combined_results = email_results.union(name_results)
         return combined_results.order_by(orderBy)
 
-    def findTeam(self, text=""):
+    def findTeam(self, text="", preload_for_api=False):
         """See `IPersonSet`."""
         orderBy = Person._sortingColumnsForSetOperations
         text = ensure_unicode(text)
@@ -3670,8 +3670,13 @@
         email_results = store.find(Person, email_query).order_by()
         name_query = self._teamNameQuery(text)
         name_results = store.find(Person, name_query).order_by()
-        combined_results = email_results.union(name_results)
-        return combined_results.order_by(orderBy)
+        result = email_results.union(name_results).order_by(orderBy)
+        if preload_for_api:
+            def preload(people):
+                list(self.getPrecachedPersonsFromIDs(
+                    [person.id for person in people], need_api=True))
+            result = DecoratedResultSet(result, pre_iter_hook=preload)
+        return result
 
     def get(self, personid):
         """See `IPersonSet`."""
@@ -3774,9 +3779,9 @@
 
     def getPrecachedPersonsFromIDs(
         self, person_ids, need_api=False, need_karma=False,
-        need_ubuntu_coc=False, need_location=False, need_archive=False,
-        need_preferred_email=False, need_validity=False,
-        need_icon=False):
+        need_ubuntu_coc=False, need_teamowner=False, need_location=False,
+        need_archive=False, need_preferred_email=False,
+        need_validity=False, need_icon=False):
         """See `IPersonSet`."""
         person_ids = set(person_ids)
         person_ids.discard(None)
@@ -3786,16 +3791,16 @@
         conditions = [
             Person.id.is_in(person_ids)]
         return self._getPrecachedPersons(
-            origin, conditions, need_api=need_api,
-            need_karma=need_karma, need_ubuntu_coc=need_ubuntu_coc,
+            origin, conditions, need_api=need_api, need_karma=need_karma,
+            need_ubuntu_coc=need_ubuntu_coc, need_teamowner=need_teamowner,
             need_location=need_location, need_archive=need_archive,
             need_preferred_email=need_preferred_email,
             need_validity=need_validity, need_icon=need_icon)
 
     def _getPrecachedPersons(
         self, origin, conditions, store=None, need_api=False,
-        need_karma=False, need_ubuntu_coc=False, need_location=False,
-        need_archive=False, need_preferred_email=False,
+        need_karma=False, need_ubuntu_coc=False, need_teamowner=False,
+        need_location=False, need_archive=False, need_preferred_email=False,
         need_validity=False, need_icon=False):
         """Lookup all members of the team with optional precaching.
 
@@ -3884,6 +3889,10 @@
         columns = tuple(columns)
         raw_result = store.using(*origin).find(columns, conditions)
 
+        def preload_for_people(rows):
+            if need_teamowner or need_api:
+                bulk.load(Person, [row[0].teamownerID for row in rows])
+
         def prepopulate_person(row):
             result = row[0]
             cache = get_property_cache(result)
@@ -3923,6 +3932,7 @@
                 decorator(result, column)
             return result
         return DecoratedResultSet(raw_result,
+            pre_iter_hook=preload_for_people,
             result_decorator=prepopulate_person)
 
 

=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2014-01-15 06:54:12 +0000
+++ lib/lp/registry/tests/test_personset.py	2014-11-17 23:51:02 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+import transaction
 
 from testtools.matchers import LessThan
 from zope.component import getUtility
@@ -110,13 +111,15 @@
         # query to load all the extraneous data. Accessing the
         # attributes should then cause zero queries.
         person_ids = [self.factory.makePerson().id for i in range(3)]
+        person_ids += [self.factory.makeTeam().id for i in range(3)]
+        transaction.commit()
 
         with StormStatementRecorder() as recorder:
             persons = list(self.person_set.getPrecachedPersonsFromIDs(
                 person_ids, need_karma=True, need_ubuntu_coc=True,
-                need_location=True, need_archive=True,
+                need_teamowner=True, need_location=True, need_archive=True,
                 need_preferred_email=True, need_validity=True))
-        self.assertThat(recorder, HasQueryCount(LessThan(2)))
+        self.assertThat(recorder, HasQueryCount(LessThan(3)))
 
         with StormStatementRecorder() as recorder:
             for person in persons:
@@ -126,6 +129,7 @@
                 person.location,
                 person.archive
                 person.preferredemail
+                person.teamowner
         self.assertThat(recorder, HasQueryCount(LessThan(1)))
 
     def test_getPrecachedPersonsFromIDs_is_ubuntu_coc_signer(self):