Merge lp:~cjwatson/launchpad/preload-git-landing-targets-candidates into lp:launchpad

[Colin Watson]

Preload {GitRepository,GitRef}.{landing_candidates,landing_targets}.

This is in line with similar handling in Branch.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/_schema_circular_imports.py'
--- lib/lp/_schema_circular_imports.py	2016-11-11 15:10:55 +0000
+++ lib/lp/_schema_circular_imports.py	2016-11-11 15:10:56 +0000
@@ -566,8 +566,10 @@
     IGitRef, 'createMergeProposal', 'merge_target', IGitRef)
 patch_plain_parameter_type(
     IGitRef, 'createMergeProposal', 'merge_prerequisite', IGitRef)
-patch_collection_property(IGitRef, 'landing_targets', IBranchMergeProposal)
-patch_collection_property(IGitRef, 'landing_candidates', IBranchMergeProposal)
+patch_collection_property(
+    IGitRef, '_api_landing_targets', IBranchMergeProposal)
+patch_collection_property(
+    IGitRef, '_api_landing_candidates', IBranchMergeProposal)
 patch_collection_property(IGitRef, 'dependent_landings', IBranchMergeProposal)
 patch_entry_return_type(IGitRef, 'createMergeProposal', IBranchMergeProposal)
 patch_collection_return_type(
@@ -581,9 +583,9 @@
 patch_entry_return_type(IGitRepository, 'getSubscription', IGitSubscription)
 patch_reference_property(IGitRepository, 'code_import', ICodeImport)
 patch_collection_property(
-    IGitRepository, 'landing_targets', IBranchMergeProposal)
+    IGitRepository, '_api_landing_targets', IBranchMergeProposal)
 patch_collection_property(
-    IGitRepository, 'landing_candidates', IBranchMergeProposal)
+    IGitRepository, '_api_landing_candidates', IBranchMergeProposal)
 patch_collection_property(
     IGitRepository, 'dependent_landings', IBranchMergeProposal)
 

=== modified file 'lib/lp/code/browser/gitref.py'
--- lib/lp/code/browser/gitref.py	2016-10-14 16:16:18 +0000
+++ lib/lp/code/browser/gitref.py	2016-11-11 15:10:56 +0000
@@ -133,12 +133,14 @@
     @cachedproperty
     def landing_targets(self):
         """Return a filtered list of landing targets."""
-        return latest_proposals_for_each_branch(self.context.landing_targets)
+        targets = self.context.getPrecachedLandingTargets(self.user)
+        return latest_proposals_for_each_branch(targets)
 
     @cachedproperty
     def landing_candidates(self):
         """Return a decorated list of landing candidates."""
-        return [proposal for proposal in self.context.landing_candidates
+        candidates = self.context.getPrecachedLandingCandidates(self.user)
+        return [proposal for proposal in candidates
                 if check_permission("launchpad.View", proposal)]
 
     def _getBranchCountText(self, count):

=== modified file 'lib/lp/code/browser/tests/test_gitref.py'
--- lib/lp/code/browser/tests/test_gitref.py	2016-09-07 11:12:58 +0000
+++ lib/lp/code/browser/tests/test_gitref.py	2016-11-11 15:10:56 +0000
@@ -12,6 +12,8 @@
 from BeautifulSoup import BeautifulSoup
 import pytz
 import soupmatchers
+from storm.store import Store
+from testtools.matchers import Equals
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -23,9 +25,11 @@
 from lp.testing import (
     admin_logged_in,
     BrowserTestCase,
+    StormStatementRecorder,
     )
 from lp.testing.dbuser import dbuser
 from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import (
     extract_text,
     find_tags_by_class,
@@ -220,3 +224,35 @@
                     soupmatchers.Tag(
                         'all commits link', 'a', text='All commits',
                         attrs={'href': expected_url}))))
+
+    def test_query_count_landing_candidates(self):
+        project = self.factory.makeProduct()
+        [ref] = self.factory.makeGitRefs(target=project)
+        for i in range(10):
+            self.factory.makeBranchMergeProposalForGit(target_ref=ref)
+        [source] = self.factory.makeGitRefs(target=project)
+        [prereq] = self.factory.makeGitRefs(target=project)
+        self.factory.makeBranchMergeProposalForGit(
+            source_ref=source, target_ref=ref, prerequisite_ref=prereq)
+        Store.of(ref).flush()
+        Store.of(ref).invalidate()
+        view = create_view(ref, '+index')
+        with StormStatementRecorder() as recorder:
+            view.landing_candidates
+        self.assertThat(recorder, HasQueryCount(Equals(11)))
+
+    def test_query_count_landing_targets(self):
+        project = self.factory.makeProduct()
+        [ref] = self.factory.makeGitRefs(target=project)
+        for i in range(10):
+            self.factory.makeBranchMergeProposalForGit(source_ref=ref)
+        [target] = self.factory.makeGitRefs(target=project)
+        [prereq] = self.factory.makeGitRefs(target=project)
+        self.factory.makeBranchMergeProposalForGit(
+            source_ref=ref, target_ref=target, prerequisite_ref=prereq)
+        Store.of(ref).flush()
+        Store.of(ref).invalidate()
+        view = create_view(ref, '+index')
+        with StormStatementRecorder() as recorder:
+            view.landing_targets
+        self.assertThat(recorder, HasQueryCount(Equals(11)))

=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py	2016-11-09 17:18:21 +0000
+++ lib/lp/code/interfaces/gitref.py	2016-11-11 15:10:56 +0000
@@ -221,22 +221,34 @@
         and their subscriptions.
         """
 
-    landing_targets = exported(CollectionField(
-        title=_("Landing targets"),
-        description=_(
-            "A collection of the merge proposals where this reference is the "
-            "source."),
-        readonly=True,
-        # Really IBranchMergeProposal, patched in _schema_circular_imports.py.
-        value_type=Reference(Interface)))
-    landing_candidates = exported(CollectionField(
-        title=_("Landing candidates"),
-        description=_(
-            "A collection of the merge proposals where this reference is the "
-            "target."),
-        readonly=True,
-        # Really IBranchMergeProposal, patched in _schema_circular_imports.py.
-        value_type=Reference(Interface)))
+    landing_targets = Attribute(
+        "A collection of the merge proposals where this reference is "
+        "the source.")
+    _api_landing_targets = exported(
+        CollectionField(
+            title=_("Landing targets"),
+            description=_(
+                "A collection of the merge proposals where this reference is "
+                "the source."),
+            readonly=True,
+            # Really IBranchMergeProposal, patched in
+            # _schema_circular_imports.py.
+            value_type=Reference(Interface)),
+        exported_as="landing_targets")
+    landing_candidates = Attribute(
+        "A collection of the merge proposals where this reference is "
+        "the target.")
+    _api_landing_candidates = exported(
+        CollectionField(
+            title=_("Landing candidates"),
+            description=_(
+                "A collection of the merge proposals where this reference is "
+                "the target."),
+            readonly=True,
+            # Really IBranchMergeProposal, patched in
+            # _schema_circular_imports.py.
+            value_type=Reference(Interface)),
+        exported_as="landing_candidates")
     dependent_landings = exported(CollectionField(
         title=_("Dependent landings"),
         description=_(
@@ -246,6 +258,18 @@
         # Really IBranchMergeProposal, patched in _schema_circular_imports.py.
         value_type=Reference(Interface)))
 
+    def getPrecachedLandingTargets(user):
+        """Return precached landing targets.
+
+        Target and prerequisite repositories are preloaded.
+        """
+
+    def getPrecachedLandingCandidates(user):
+        """Return precached landing candidates.
+
+        Source and prerequisite repositories are preloaded.
+        """
+
     # XXX cjwatson 2015-04-16: Rename in line with landing_targets above
     # once we have a better name.
     def addLandingTarget(registrant, merge_target, merge_prerequisite=None,

=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py	2016-10-14 15:08:36 +0000
+++ lib/lp/code/interfaces/gitrepository.py	2016-11-11 15:10:56 +0000
@@ -476,22 +476,34 @@
         and their subscriptions.
         """
 
-    landing_targets = exported(CollectionField(
-        title=_("Landing targets"),
-        description=_(
-            "A collection of the merge proposals where this repository is the "
-            "source."),
-        readonly=True,
-        # Really IBranchMergeProposal, patched in _schema_circular_imports.py.
-        value_type=Reference(Interface)))
-    landing_candidates = exported(CollectionField(
-        title=_("Landing candidates"),
-        description=_(
-            "A collection of the merge proposals where this repository is the "
-            "target."),
-        readonly=True,
-        # Really IBranchMergeProposal, patched in _schema_circular_imports.py.
-        value_type=Reference(Interface)))
+    landing_targets = Attribute(
+        "A collection of the merge proposals where this repository is "
+        "the source.")
+    _api_landing_targets = exported(
+        CollectionField(
+            title=_("Landing targets"),
+            description=_(
+                "A collection of the merge proposals where this repository is "
+                "the source."),
+            readonly=True,
+            # Really IBranchMergeProposal, patched in
+            # _schema_circular_imports.py.
+            value_type=Reference(Interface)),
+        exported_as="landing_targets")
+    landing_candidates = Attribute(
+        "A collection of the merge proposals where this repository is "
+        "the target.")
+    _api_landing_candidates = exported(
+        CollectionField(
+            title=_("Landing candidates"),
+            description=_(
+                "A collection of the merge proposals where this repository is "
+                "the target."),
+            readonly=True,
+            # Really IBranchMergeProposal, patched in
+            # _schema_circular_imports.py.
+            value_type=Reference(Interface)),
+        exported_as="landing_candidates")
     dependent_landings = exported(CollectionField(
         title=_("Dependent landings"),
         description=_(
@@ -501,6 +513,18 @@
         # Really IBranchMergeProposal, patched in _schema_circular_imports.py.
         value_type=Reference(Interface)))
 
+    def getPrecachedLandingTargets(user):
+        """Return precached landing targets.
+
+        Target and prerequisite repositories are preloaded.
+        """
+
+    def getPrecachedLandingCandidates(user):
+        """Return precached landing candidates.
+
+        Source and prerequisite repositories are preloaded.
+        """
+
     def getMergeProposalByID(id):
         """Return this repository's merge proposal with this id, or None."""
 

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2016-11-09 17:18:21 +0000
+++ lib/lp/code/model/gitref.py	2016-11-11 15:10:56 +0000
@@ -8,6 +8,7 @@
     ]
 
 from datetime import datetime
+from functools import partial
 import json
 from urllib import quote_plus
 from urlparse import urlsplit
@@ -51,7 +52,6 @@
     BranchMergeProposalGetter,
     )
 from lp.services.config import config
-from lp.services.database.bulk import load_related
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.enumcol import EnumCol
@@ -59,6 +59,7 @@
 from lp.services.database.stormbase import StormBase
 from lp.services.features import getFeatureFlag
 from lp.services.memcache.interfaces import IMemcacheClient
+from lp.services.webapp.interfaces import ILaunchBag
 
 
 class GitRefMixin:
@@ -194,25 +195,34 @@
             BranchMergeProposal.source_git_repository == self.repository,
             BranchMergeProposal.source_git_path == self.path)
 
+    def getPrecachedLandingTargets(self, user):
+        """See `IGitRef`."""
+        loader = partial(BranchMergeProposal.preloadDataForBMPs, user=user)
+        return DecoratedResultSet(self.landing_targets, pre_iter_hook=loader)
+
+    @property
+    def _api_landing_targets(self):
+        return self.getPrecachedLandingTargets(getUtility(ILaunchBag).user)
+
     @property
     def landing_candidates(self):
         """See `IGitRef`."""
-        # Circular import.
-        from lp.code.model.gitrepository import GitRepository
-
-        result = Store.of(self).find(
+        return Store.of(self).find(
             BranchMergeProposal,
             BranchMergeProposal.target_git_repository == self.repository,
             BranchMergeProposal.target_git_path == self.path,
             Not(BranchMergeProposal.queue_status.is_in(
                 BRANCH_MERGE_PROPOSAL_FINAL_STATES)))
 
-        def eager_load(rows):
-            load_related(
-                GitRepository, rows,
-                ["source_git_repositoryID", "prerequisite_git_repositoryID"])
+    def getPrecachedLandingCandidates(self, user):
+        """See `IGitRef`."""
+        loader = partial(BranchMergeProposal.preloadDataForBMPs, user=user)
+        return DecoratedResultSet(
+            self.landing_candidates, pre_iter_hook=loader)
 
-        return DecoratedResultSet(result, pre_iter_hook=eager_load)
+    @property
+    def _api_landing_candidates(self):
+        return self.getPrecachedLandingCandidates(getUtility(ILaunchBag).user)
 
     @property
     def dependent_landings(self):

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2016-10-14 15:08:58 +0000
+++ lib/lp/code/model/gitrepository.py	2016-11-11 15:10:56 +0000
@@ -155,6 +155,7 @@
     get_property_cache,
     )
 from lp.services.webapp.authorization import available_with_permission
+from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webhooks.interfaces import IWebhookSet
 from lp.services.webhooks.model import WebhookTargetMixin
 from lp.snappy.interfaces.snap import ISnapSet
@@ -887,6 +888,15 @@
             BranchMergeProposal,
             BranchMergeProposal.source_git_repository == self)
 
+    def getPrecachedLandingTargets(self, user):
+        """See `IGitRef`."""
+        loader = partial(BranchMergeProposal.preloadDataForBMPs, user=user)
+        return DecoratedResultSet(self.landing_targets, pre_iter_hook=loader)
+
+    @property
+    def _api_landing_targets(self):
+        return self.getPrecachedLandingTargets(getUtility(ILaunchBag).user)
+
     def getActiveLandingTargets(self, paths):
         """Merge proposals not in final states where these refs are source."""
         return Store.of(self).find(
@@ -905,6 +915,16 @@
             Not(BranchMergeProposal.queue_status.is_in(
                 BRANCH_MERGE_PROPOSAL_FINAL_STATES)))
 
+    def getPrecachedLandingCandidates(self, user):
+        """See `IGitRef`."""
+        loader = partial(BranchMergeProposal.preloadDataForBMPs, user=user)
+        return DecoratedResultSet(
+            self.landing_candidates, pre_iter_hook=loader)
+
+    @property
+    def _api_landing_candidates(self):
+        return self.getPrecachedLandingCandidates(getUtility(ILaunchBag).user)
+
     def getActiveLandingCandidates(self, paths):
         """Merge proposals not in final states where these refs are target."""
         return Store.of(self).find(

=== modified file 'lib/lp/code/model/tests/test_gitref.py'
--- lib/lp/code/model/tests/test_gitref.py	2016-09-07 11:12:58 +0000
+++ lib/lp/code/model/tests/test_gitref.py	2016-11-11 15:10:56 +0000
@@ -18,6 +18,7 @@
     EndsWith,
     Equals,
     Is,
+    LessThan,
     MatchesListwise,
     MatchesStructure,
     )
@@ -36,6 +37,7 @@
     ANONYMOUS,
     api_url,
     person_logged_in,
+    record_two_runs,
     TestCaseWithFactory,
     verifyObject,
     )
@@ -43,6 +45,7 @@
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
     )
+from lp.testing.matchers import HasQueryCount
 from lp.testing.pages import webservice_for_person
 
 
@@ -461,6 +464,30 @@
         self.assertThat(
             landing_candidates["entries"][0]["self_link"], EndsWith(bmp_url))
 
+    def test_landing_candidates_constant_queries(self):
+        project = self.factory.makeProduct()
+        with person_logged_in(project.owner):
+            [trunk] = self.factory.makeGitRefs(target=project)
+            trunk_url = api_url(trunk)
+            webservice = webservice_for_person(
+                project.owner, permission=OAuthPermission.WRITE_PRIVATE)
+
+        def create_mp():
+            with admin_logged_in():
+                [ref] = self.factory.makeGitRefs(
+                    target=project,
+                    information_type=InformationType.PRIVATESECURITY)
+                self.factory.makeBranchMergeProposalForGit(
+                    source_ref=ref, target_ref=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)))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
     def test_landing_targets(self):
         bmp_db = self.factory.makeBranchMergeProposalForGit()
         with person_logged_in(bmp_db.registrant):
@@ -476,6 +503,30 @@
         self.assertThat(
             landing_targets["entries"][0]["self_link"], EndsWith(bmp_url))
 
+    def test_landing_targets_constant_queries(self):
+        project = self.factory.makeProduct()
+        with person_logged_in(project.owner):
+            [source] = self.factory.makeGitRefs(target=project)
+            source_url = api_url(source)
+            webservice = webservice_for_person(
+                project.owner, permission=OAuthPermission.WRITE_PRIVATE)
+
+        def create_mp():
+            with admin_logged_in():
+                [ref] = self.factory.makeGitRefs(
+                    target=project,
+                    information_type=InformationType.PRIVATESECURITY)
+                self.factory.makeBranchMergeProposalForGit(
+                    source_ref=source, target_ref=ref)
+
+        def list_mps():
+            webservice.get(source_url + "/landing_targets")
+
+        list_mps()
+        recorder1, recorder2 = record_two_runs(list_mps, create_mp, 2)
+        self.assertThat(recorder1, HasQueryCount(LessThan(30)))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
     def test_dependent_landings(self):
         [ref] = self.factory.makeGitRefs()
         bmp_db = self.factory.makeBranchMergeProposalForGit(

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2016-10-14 16:16:18 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2016-11-11 15:10:56 +0000
@@ -20,6 +20,7 @@
 from storm.store import Store
 from testtools.matchers import (
     EndsWith,
+    LessThan,
     MatchesSetwise,
     MatchesStructure,
     )
@@ -2800,6 +2801,31 @@
         self.assertThat(
             landing_candidates["entries"][0]["self_link"], EndsWith(bmp_url))
 
+    def test_landing_candidates_constant_queries(self):
+        project = self.factory.makeProduct()
+        with person_logged_in(project.owner):
+            repository = self.factory.makeGitRepository(target=project)
+            repository_url = api_url(repository)
+            webservice = webservice_for_person(
+                project.owner, permission=OAuthPermission.WRITE_PRIVATE)
+
+        def create_mp():
+            with admin_logged_in():
+                [target] = self.factory.makeGitRefs(repository=repository)
+                [source] = self.factory.makeGitRefs(
+                    target=project,
+                    information_type=InformationType.PRIVATESECURITY)
+                self.factory.makeBranchMergeProposalForGit(
+                    source_ref=source, target_ref=target)
+
+        def list_mps():
+            webservice.get(repository_url + "/landing_candidates")
+
+        list_mps()
+        recorder1, recorder2 = record_two_runs(list_mps, create_mp, 2)
+        self.assertThat(recorder1, HasQueryCount(LessThan(40)))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
     def test_landing_targets(self):
         bmp_db = self.factory.makeBranchMergeProposalForGit()
         with person_logged_in(bmp_db.registrant):
@@ -2815,6 +2841,31 @@
         self.assertThat(
             landing_targets["entries"][0]["self_link"], EndsWith(bmp_url))
 
+    def test_landing_targets_constant_queries(self):
+        project = self.factory.makeProduct()
+        with person_logged_in(project.owner):
+            repository = self.factory.makeGitRepository(target=project)
+            repository_url = api_url(repository)
+            webservice = webservice_for_person(
+                project.owner, permission=OAuthPermission.WRITE_PRIVATE)
+
+        def create_mp():
+            with admin_logged_in():
+                [source] = self.factory.makeGitRefs(repository=repository)
+                [target] = self.factory.makeGitRefs(
+                    target=project,
+                    information_type=InformationType.PRIVATESECURITY)
+                self.factory.makeBranchMergeProposalForGit(
+                    source_ref=source, target_ref=target)
+
+        def list_mps():
+            webservice.get(repository_url + "/landing_targets")
+
+        list_mps()
+        recorder1, recorder2 = record_two_runs(list_mps, create_mp, 2)
+        self.assertThat(recorder1, HasQueryCount(LessThan(30)))
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
     def test_dependent_landings(self):
         [ref] = self.factory.makeGitRefs()
         bmp_db = self.factory.makeBranchMergeProposalForGit(