Merge lp:~rvb/launchpad/activereviews-bug-867941-eagerload3 into lp:launchpad

[Raphaël Badin]

[r=bac][bug=867941] Preload the chain of stacked on branches.

This branch is (hopefully) the last one in a series aimed at improving the performance of +activereviews. This branch improves the preloading of the chain of stacked on branches. It uses a recursive CTE to fetch in one query all the branches on which the branches to be displayed in {person,product}:activereviews are stacked on.

Two things I'd be happy to have a reviewer's option about:
- since the list of branches provided to preloadVisibleStackedOnBranches is a list of visible branches, is it necessary to make sure that all the returned branches are also visible? I think that If a branch is stacked on a non visible branch then it won't be visible ifself.
- I would be happy to avoid using the name 'column1' (which is the default postgresql name for columns created by VALUES expressions but is not a sql standard) in 'SELECT column1 as id FROM (VALUES %s) AS temp[...]' but I could not find a way to do this. Here is a working query if you are wanting to give a try at modifying it: http://paste.ubuntu.com/749381/.

=Tests=
(changed)
/bin/test -vvc test_branchmergeproposallisting test_product_activereviews_query_count
(added)
/bin/test -vvc test_branchcollection test_preloadVisibleStackedOnBranches_visible_private_branches
/bin/test -vvc test_branchcollection test_preloadVisibleStackedOnBranches_anon_public_branches
/bin/test -vvc test_branchcollection test_preloadVisibleStackedOnBranches_non_anon_public_branches

=Q/A=
https://code.qastaging.launchpad.net/~ubuntu-branches/+activereviews should not issue a single repeated query.

[Brad Crittenden (bac)]

Hi Raphaël,

* For createStackedOnChain(self, product) it might be nice to add another argument for the number of branches, defaulting to 5.

* Can branch_ids = map(itemgetter(0), result.get_all()) be rewritten as
           branch_ids = [res[0] for res in result.get_all()] ? If so, all things being equal, I think it is more readable.

* Nice use of 'partial'. I learned something new!

* Actually now that I see createBranchChain it looks like createdStackedOnChain is redundant. Care to promote createBranchChain to the factory.py and re-use it?

* As to your question about visibility of stacked on branches, if you can write a test to prove your assertion then I'd be happy for you to delete the extra step. But the test must be there to ensure we don't have a regression that will allow private branches to leak.

* Sorry I have no ideas on your 'column1' question. Perhaps you can ask stub later.

review: Approve

[Brad Crittenden (bac)]

review: Approve (code)

[Raphaël Badin (rvb)]

Thanks for the review Brad!

> * Can branch_ids = map(itemgetter(0), result.get_all()) be rewritten as
> branch_ids = [res[0] for res in result.get_all()] ? If so, all
> things being equal, I think it is more readable.

True. Although you will admit that using map/itemgetter is cooler ;).

> * Actually now that I see createBranchChain it looks like
> createdStackedOnChain is redundant. Care to promote createBranchChain to the
> factory.py and re-use it?

Good idea.

> * As to your question about visibility of stacked on branches, if you can
> write a test to prove your assertion then I'd be happy for you to delete the
> extra step. But the test must be there to ensure we don't have a regression
> that will allow private branches to leak.

Good point, I'll keep the assertion for now.

> * Sorry I have no ideas on your 'column1' question. Perhaps you can ask stub
> later.

I suppose it'd good enough to be landed as is but I'll ask stub/jtv.

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py	2011-11-23 14:36:15 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py	2011-11-25 22:09:49 +0000
@@ -449,7 +449,9 @@
         self.assertThat(recorder2, HasQueryCount(Equals(recorder1.count)))
 
     def createProductBMP(self, product):
-        bmp = self.factory.makeBranchMergeProposal(product=product)
+        target_branch = self.factory.makeStackedOnBranchChain(product=product)
+        bmp = self.factory.makeBranchMergeProposal(
+            product=product, target_branch=target_branch)
         self.setupBMP(bmp)
         return bmp
 
 
=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-11-24 12:36:57 +0000
+++ lib/lp/code/model/branchcollection.py	2011-11-25 22:09:49 +0000
@@ -9,6 +9,10 @@
     ]
 
 from collections import defaultdict
+from functools import partial
+from operator import (
+    attrgetter,
+    )
 
 from lazr.restful.utils import safe_hasattr
 from storm.expr import (
@@ -29,6 +33,7 @@
 from zope.component import getUtility
 from zope.interface import implements
 
+from canonical.database.sqlbase import quote
 from canonical.launchpad.components.decoratedresultset import (
     DecoratedResultSet,
     )
@@ -122,6 +127,7 @@
         if exclude_from_search is None:
             exclude_from_search = []
         self._exclude_from_search = exclude_from_search
+        self._user = None
 
     def count(self):
         """See `IBranchCollection`."""
@@ -218,6 +224,43 @@
             With("candidate_branches", SQL("SELECT id from scope_branches"))]
 
     @staticmethod
+    def preloadVisibleStackedOnBranches(branches, user=None):
+        """Preload the chains of stacked on branches related to the given list
+        of branches. Only the branches visible for the given user are
+        preloaded/returned.
+
+        """
+        if len(branches) == 0:
+            return
+        store = IStore(Branch)
+        result = store.execute("""
+            WITH RECURSIVE stacked_on_branches_ids AS (
+                SELECT column1 as id FROM (VALUES %s) AS temp
+                UNION
+                SELECT DISTINCT branch.stacked_on
+                FROM stacked_on_branches_ids, Branch AS branch
+                WHERE
+                    branch.id = stacked_on_branches_ids.id AND
+                    branch.stacked_on IS NOT NULL
+            )
+            SELECT id from stacked_on_branches_ids
+            """ % ', '.join(
+                ["(%s)" % quote(id)
+                 for id in map(attrgetter('id'), branches)]))
+        branch_ids = [res[0] for res in result.get_all()]
+        # Not really sure this is useful: if a given branch is visible by a
+        # user, then I think it means that the whole chain of branches on
+        # which is is stacked on is visible by this user
+        expressions = [Branch.id.is_in(branch_ids)]
+        if user is None:
+            collection = AnonymousBranchCollection(
+                branch_filter_expressions=expressions)
+        else:
+            collection = VisibleBranchCollection(
+                user=user, branch_filter_expressions=expressions)
+        return list(collection.getBranches())
+
+    @staticmethod
     def preloadDataForBranches(branches):
         """Preload branches cached associated product series and
         suite source packages."""
@@ -291,8 +334,9 @@
             for_branches is not None or
             target_branch is not None or
             merged_revnos is not None):
-            return self._naiveGetMergeProposals(statuses, for_branches,
-                target_branch, merged_revnos, eager_load=eager_load)
+            return self._naiveGetMergeProposals(
+                statuses, for_branches, target_branch, merged_revnos,
+                eager_load=eager_load)
         else:
             # When examining merge proposals in a scope, this is a moderately
             # effective set of constrained queries. It is not effective when
@@ -333,9 +377,9 @@
         if not eager_load:
             return resultset
         else:
-            return DecoratedResultSet(
-                resultset,
-                pre_iter_hook=BranchMergeProposal.preloadDataForBMPs)
+            loader = partial(
+                BranchMergeProposal.preloadDataForBMPs, user=self._user)
+            return DecoratedResultSet(resultset, pre_iter_hook=loader)
 
     def _scopedGetMergeProposals(self, statuses, eager_load=False):
         scope_tables = [Branch] + self._tables.values()
@@ -365,9 +409,9 @@
         if not eager_load:
             return resultset
         else:
-            return DecoratedResultSet(
-                resultset,
-                pre_iter_hook=BranchMergeProposal.preloadDataForBMPs)
+            loader = partial(
+                BranchMergeProposal.preloadDataForBMPs, user=self._user)
+            return DecoratedResultSet(resultset, pre_iter_hook=loader)
 
     def getMergeProposalsForPerson(self, person, status=None,
                                    eager_load=False):
@@ -381,9 +425,9 @@
         if not eager_load:
             return resultset
         else:
-            return DecoratedResultSet(
-                resultset,
-                pre_iter_hook=BranchMergeProposal.preloadDataForBMPs)
+            loader = partial(
+                BranchMergeProposal.preloadDataForBMPs, user=self._user)
+            return DecoratedResultSet(resultset, pre_iter_hook=loader)
 
     def getMergeProposalsForReviewer(self, reviewer, status=None):
         """See `IBranchCollection`."""
 
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2011-11-24 13:26:16 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2011-11-25 22:09:49 +0000
@@ -911,7 +911,7 @@
         return [range_ for range_, diff in zip(ranges, diffs) if diff is None]
 
     @staticmethod
-    def preloadDataForBMPs(branch_merge_proposals):
+    def preloadDataForBMPs(branch_merge_proposals, user):
         # Utility to load the data related to a list of bmps.
         # Circular imports.
         from lp.code.model.branch import Branch
@@ -933,9 +933,8 @@
                 "target_branchID", "prerequisite_branchID",
                 "source_branchID"))
         # The stacked on branches are used to check branch visibility.
-        # This is only temporary because we should fetch the whole chain
-        # of stacked on branches instead of only the first level.
-        load_related(Branch, branches, ["stacked_onID"])
+        GenericBranchCollection.preloadVisibleStackedOnBranches(
+            branches, user)
 
         if len(branches) == 0:
             return
@@ -951,8 +950,6 @@
             [preview_diff_and_diff[0] for preview_diff_and_diff
                 in preview_diffs_and_diffs])
 
-        GenericBranchCollection.preloadDataForBranches(branches)
-
         # Add source branch owners' to the list of pre-loaded persons.
         person_ids.update(
             branch.ownerID for branch in branches
@@ -966,6 +963,7 @@
         load_related(SourcePackageName, branches, ['sourcepackagenameID'])
         load_related(DistroSeries, branches, ['distroseriesID'])
         load_related(Product, branches, ['productID'])
+        GenericBranchCollection.preloadDataForBranches(branches)
 
 
 class BranchMergeProposalGetter:
 
=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py	2011-10-10 00:03:00 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py	2011-11-25 22:09:49 +0000
@@ -151,6 +151,51 @@
             self.store, [Branch.product == branch.product])
         self.assertEqual(1, collection.count())
 
+    def test_preloadVisibleStackedOnBranches_visible_private_branches(self):
+        person = self.factory.makePerson()
+        branch_number = 2
+        depth = 3
+        # Create private branches person can see.
+        branches = []
+        for i in range(branch_number):
+            branches.append(
+                self.factory.makeStackedOnBranchChain(
+                    owner=person, private=True, depth=depth))
+        with person_logged_in(person):
+            all_branches = (
+                GenericBranchCollection.preloadVisibleStackedOnBranches(
+                    branches, person))
+        self.assertEqual(len(all_branches), branch_number * depth)
+
+    def test_preloadVisibleStackedOnBranches_anon_public_branches(self):
+        branch_number = 2
+        depth = 3
+        # Create public branches.
+        branches = []
+        for i in range(branch_number):
+            branches.append(
+                self.factory.makeStackedOnBranchChain(
+                    private=False, depth=depth))
+        all_branches = (
+            GenericBranchCollection.preloadVisibleStackedOnBranches(branches))
+        self.assertEqual(len(all_branches), branch_number * depth)
+
+    def test_preloadVisibleStackedOnBranches_non_anon_public_branches(self):
+        person = self.factory.makePerson()
+        branch_number = 2
+        depth = 3
+        # Create public branches.
+        branches = []
+        for i in range(branch_number):
+            branches.append(
+                self.factory.makeStackedOnBranchChain(
+                    owner=person, private=False, depth=depth))
+        with person_logged_in(person):
+            all_branches = (
+                GenericBranchCollection.preloadVisibleStackedOnBranches(
+                    branches, person))
+        self.assertEqual(len(all_branches), branch_number * depth)
+
 
 class TestBranchCollectionFilters(TestCaseWithFactory):
 
 
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-11-22 22:07:39 +0000
+++ lib/lp/testing/factory.py	2011-11-25 22:09:49 +0000
@@ -1084,6 +1084,12 @@
             owner=owner, name=name, title=title, time_zone=time_zone,
             time_starts=time_starts, time_ends=time_ends, summary=summary)
 
+    def makeStackedOnBranchChain(self, depth=5, **kwargs):
+        branch = None
+        for i in xrange(depth):
+            branch = self.makeAnyBranch(stacked_on=branch, **kwargs)
+        return branch
+
     def makeBranch(self, branch_type=None, owner=None,
                    name=None, product=_DEFAULT, url=_DEFAULT, registrant=None,
                    private=False, stacked_on=None, sourcepackage=None,