launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/getMergeProposals-timeout into lp:launchpad.

Commit message:
Make Person.getMergeProposals have a constant query count on the webservice.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1619772 in Launchpad itself: "user.getMergeProposals(status="Merged") consistently timing out for some users"
  https://bugs.launchpad.net/launchpad/+bug/1619772

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/getMergeProposals-timeout/+merge/305065

Make Person.getMergeProposals have a constant query count on the webservice.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/getMergeProposals-timeout into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/hasbranches.py'
--- lib/lp/code/interfaces/hasbranches.py	2015-05-12 17:30:40 +0000
+++ lib/lp/code/interfaces/hasbranches.py	2016-09-07 03:14:52 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface definitions for IHas<code related bits>."""
@@ -87,15 +87,17 @@
         status=List(
             title=_("A list of merge proposal statuses to filter by."),
             value_type=Choice(vocabulary=BranchMergeProposalStatus)))
-    @call_with(visible_by_user=REQUEST_USER)
+    @call_with(visible_by_user=REQUEST_USER, eager_load=True)
     @operation_returns_collection_of(Interface)  # Really IBranchMergeProposal.
     @export_read_operation()
     @operation_for_version('beta')
-    def getMergeProposals(status=None, visible_by_user=None):
+    def getMergeProposals(status=None, visible_by_user=None, eager_load=False):
         """Returns all merge proposals of a given status.
 
         :param status: A list of statuses to filter with.
         :param visible_by_user: Normally the user who is asking.
+        :param eager_load: If True, load related objects for the whole
+            collection.
         :returns: A list of `IBranchMergeProposal`.
         """
 

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2016-09-02 18:21:07 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2016-09-07 03:14:52 +0000
@@ -101,6 +101,7 @@
 from lp.services.config import config
 from lp.services.database.bulk import (
     load,
+    load_referencing,
     load_related,
     )
 from lp.services.database.constants import (
@@ -521,7 +522,11 @@
         dbName='superseded_by', foreignKey='BranchMergeProposal',
         notNull=False, default=None)
 
-    supersedes = Reference("<primary key>", "superseded_by", on_remote=True)
+    _supersedes = Reference("<primary key>", "superseded_by", on_remote=True)
+
+    @cachedproperty
+    def supersedes(self):
+        return self._supersedes
 
     date_created = UtcDateTimeCol(notNull=True, default=DEFAULT)
     date_review_requested = UtcDateTimeCol(notNull=False, default=None)
@@ -1288,9 +1293,21 @@
                 Desc(PreviewDiff.date_created)).config(
                     distinct=[PreviewDiff.branch_merge_proposal_id])
         load_related(Diff, preview_diffs, ['diff_id'])
+        preview_diff_map = {}
         for previewdiff in preview_diffs:
-            cache = get_property_cache(previewdiff.branch_merge_proposal)
-            cache.preview_diff = previewdiff
+            preview_diff_map[previewdiff.branch_merge_proposal_id] = (
+                previewdiff)
+        for mp in branch_merge_proposals:
+            get_property_cache(mp).preview_diff = preview_diff_map.get(mp.id)
+
+        # Preload other merge proposals that supersede these.
+        supersedes_map = {}
+        for other_mp in load_referencing(
+                BranchMergeProposal, branch_merge_proposals,
+                ['superseded_byID']):
+            supersedes_map[other_mp.superseded_byID] = other_mp
+        for mp in branch_merge_proposals:
+            get_property_cache(mp).supersedes = supersedes_map.get(mp.id)
 
         # Add source branch/repository owners' to the list of pre-loaded
         # persons.  We need the target repository owner as well; unlike

=== modified file 'lib/lp/code/model/tests/test_hasbranches.py'
--- lib/lp/code/model/tests/test_hasbranches.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/model/tests/test_hasbranches.py	2016-09-07 03:14:52 +0000
@@ -1,13 +1,24 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for classes that implement IHasBranches."""
 
 __metaclass__ = type
 
+from functools import partial
+
 from lp.code.interfaces.hasbranches import IHasBranches
-from lp.testing import TestCaseWithFactory
+from lp.services.webapp.interfaces import OAuthPermission
+from lp.testing import (
+    api_url,
+    login_person,
+    logout,
+    record_two_runs,
+    TestCaseWithFactory,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.matchers import HasQueryCount
+from lp.testing.pages import webservice_for_person
 
 
 class TestIHasBranches(TestCaseWithFactory):
@@ -29,3 +40,37 @@
         # ProjectGroups should implement IHasBranches.
         project = self.factory.makeProject()
         self.assertProvides(project, IHasBranches)
+
+
+class TestHasMergeProposalsWebservice(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_constant_query_count(self):
+        # Person.getMergeProposals has a constant query count on the
+        # webservice.
+
+        # Ensure that both runs fit in a single batch, to avoid needing to
+        # account for an extra count query.
+        self.pushConfig("launchpad", default_batch_size=10)
+
+        owner = self.factory.makePerson()
+        owner_url = api_url(owner)
+        webservice = webservice_for_person(
+            owner, permission=OAuthPermission.READ_PUBLIC)
+
+        def create_merge_proposal():
+            source_branch = self.factory.makeProductBranch(owner=owner)
+            self.factory.makeBranchMergeProposal(source_branch=source_branch)
+            [source_ref] = self.factory.makeGitRefs(owner=owner)
+            self.factory.makeBranchMergeProposalForGit(source_ref=source_ref)
+
+        def get_merge_proposals():
+            logout()
+            response = webservice.named_get(owner_url, "getMergeProposals")
+            self.assertEqual(200, response.status)
+
+        recorder1, recorder2 = record_two_runs(
+            get_merge_proposals, create_merge_proposal, 2,
+            login_method=partial(login_person, owner), record_request=True)
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))