launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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

Commit message:
Precache branch permissions in Branch.landing_candidates, fixing API timeouts.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1500576 in Launchpad itself: "Landing candidates API timeouts checking for privacy"
  https://bugs.launchpad.net/launchpad/+bug/1500576

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

Fix (or at least markedly reduce) API timeouts of Branch.landing_candidates (bug #1500576). OOPS-99645f480967e3ac2b47509394004884 shows that nearly 500 of the 600 queries are checking branch privacy, so I replaced the property with a call to getPrecachedLandingCandidates. It means using LaunchBag in the model, but it's the only option here due to lazr.restful limitations.

There are three queries per MP left over, but they're comparatively few and cheap. A test is in place to ensure it doesn't regress past those.

landing_targets and dependent_branches could arguably use the same treatment, but they're very rarely used and almost always just a couple of entries long.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1500576 into lp:launchpad.

=== modified file 'lib/lp/_schema_circular_imports.py'
--- lib/lp/_schema_circular_imports.py	2015-08-03 11:45:43 +0000
+++ lib/lp/_schema_circular_imports.py	2015-09-29 00:49:05 +0000
@@ -238,7 +238,7 @@
 patch_collection_property(IBranch, 'linked_bugs', IBug)
 patch_collection_property(IBranch, 'dependent_branches', IBranchMergeProposal)
 patch_entry_return_type(IBranch, 'getSubscription', IBranchSubscription)
-patch_collection_property(IBranch, 'landing_candidates', IBranchMergeProposal)
+patch_collection_property(IBranch, '_api_landing_candidates', IBranchMergeProposal)
 patch_collection_property(IBranch, 'landing_targets', IBranchMergeProposal)
 patch_plain_parameter_type(IBranch, 'linkBug', 'bug', IBug)
 patch_plain_parameter_type(

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2015-09-23 15:38:13 +0000
+++ lib/lp/code/interfaces/branch.py	2015-09-29 00:49:05 +0000
@@ -551,14 +551,18 @@
                 'the source branch.'),
             readonly=True,
             value_type=Reference(Interface)))
-    landing_candidates = exported(
+    landing_candidates = Attribute(
+        'A collection of the merge proposals where this branch is '
+        'the target branch.')
+    _api_landing_candidates = exported(
         CollectionField(
             title=_('Landing Candidates'),
             description=_(
                 'A collection of the merge proposals where this branch is '
                 'the target branch.'),
             readonly=True,
-            value_type=Reference(Interface)))
+            value_type=Reference(Interface)),
+        exported_as='landing_candidates')
     dependent_branches = exported(
         CollectionField(
             title=_('Dependent Branches'),

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2015-09-23 15:38:13 +0000
+++ lib/lp/code/model/branch.py	2015-09-29 00:49:05 +0000
@@ -179,6 +179,7 @@
 from lp.services.propertycache import cachedproperty
 from lp.services.webapp import urlappend
 from lp.services.webapp.authorization import check_permission
+from lp.services.webapp.interfaces import ILaunchBag
 
 
 @implementer(IBranch, IPrivacy, IInformationType)
@@ -494,6 +495,10 @@
             self.landing_candidates, pre_iter_hook=eager_load)
 
     @property
+    def _api_landing_candidates(self):
+        return self.getPrecachedLandingCandidates(getUtility(ILaunchBag).user)
+
+    @property
     def dependent_branches(self):
         """See `IBranch`."""
         return BranchMergeProposal.select("""

=== modified file 'lib/lp/code/tests/test_branch_webservice.py'
--- lib/lp/code/tests/test_branch_webservice.py	2012-09-26 07:59:35 +0000
+++ lib/lp/code/tests/test_branch_webservice.py	2015-09-29 00:49:05 +0000
@@ -4,6 +4,10 @@
 __metaclass__ = type
 
 from lazr.restfulclient.errors import BadRequest
+from testtools.matchers import (
+    Equals,
+    LessThan,
+    )
 from zope.component import getUtility
 from zope.security.management import endInteraction
 from zope.security.proxy import removeSecurityProxy
@@ -12,22 +16,64 @@
 from lp.code.interfaces.branch import IBranchSet
 from lp.code.interfaces.linkedbranch import ICanHasLinkedBranch
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.services.webapp.interfaces import OAuthPermission
 from lp.testing import (
+    admin_logged_in,
     api_url,
     launchpadlib_for,
     login_person,
     logout,
+    record_two_runs,
     run_with_login,
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.pages import webservice_for_person
+from lp.testing.matchers import HasQueryCount
+
+
+class TestBranch(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_landing_candidates_constant_queries(self):
+        target = self.factory.makeProduct()
+        login_person(target.owner)
+        trunk = self.factory.makeBranch(target=target)
+        trunk_url = api_url(trunk)
+        webservice = webservice_for_person(
+            target.owner, permission=OAuthPermission.WRITE_PRIVATE)
+        logout()
+
+        def create_mp():
+            with admin_logged_in():
+                branch = self.factory.makeBranch(
+                    target=target,
+                    stacked_on=self.factory.makeBranch(
+                        target=target,
+                        information_type=InformationType.PRIVATESECURITY),
+                    information_type=InformationType.PRIVATESECURITY)
+                self.factory.makeBranchMergeProposal(
+                    source_branch=branch, target_branch=trunk)
+
+        def list_mps():
+            webservice.get(trunk_url + '/landing_candidates')
+
+        list_mps()
+        recorder1, recorder2 = record_two_runs(list_mps, create_mp, 2)
+        self.assertThat(recorder1, HasQueryCount(LessThan(30)))
+        # Each new MP triggers a query for Person, BranchMergeProposal
+        # and PreviewDiff. Eventually they should be batched, but this
+        # at least ensures the situation doesn't get worse.
+        self.assertThat(
+            recorder2, HasQueryCount(Equals(recorder1.count + (2 * 3))))
 
 
 class TestBranchOperations(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
-    def test_createMergeProposal_fails_if_reviewers_and_review_types_are_different_sizes(self):
+    def test_createMergeProposal_fails_if_reviewers_and_types_mismatch(self):
 
         source = self.factory.makeBranch(name='rock')
         source_url = api_url(source)
@@ -105,6 +151,7 @@
         self.assertEqual(
             '~barney/myproduct/mybranch', renamed_branch.unique_name)
 
+
 class TestBranchDeletes(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer