launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #933938 in Launchpad itself: "Migrate branch visibility rules to project sharing"
  https://bugs.launchpad.net/launchpad/+bug/933938

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-933938-mps/+merge/113949

One bit of branchcollection still uses BranchSubscription for access checks: the hilariously complicated optimisation CTE in getMergeProposals. Now that access checks can be performed inline during Branch row scans we don't need the overcomplicated public/private split, so the CTE can be reduced to simply use the normal access check. This lets a lot of code evaporate.

For Launchpad itself, the old query takes 600-900ms and the new one 150-200ms.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-933938-mps/+merge/113949
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-933938-mps into lp:launchpad.

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2012-07-05 04:59:52 +0000
+++ lib/lp/code/model/branchcollection.py	2012-07-09 10:48:20 +0000
@@ -59,10 +59,7 @@
 from lp.code.model.codereviewcomment import CodeReviewComment
 from lp.code.model.codereviewvote import CodeReviewVoteReference
 from lp.code.model.seriessourcepackagebranch import SeriesSourcePackageBranch
-from lp.registry.enums import (
-    PRIVATE_INFORMATION_TYPES,
-    PUBLIC_INFORMATION_TYPES,
-    )
+from lp.registry.enums import PUBLIC_INFORMATION_TYPES
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.distroseries import DistroSeries
 from lp.registry.model.person import (
@@ -78,10 +75,7 @@
     )
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.lpstorm import IStore
-from lp.services.database.sqlbase import (
-    quote,
-    sqlvalues,
-    )
+from lp.services.database.sqlbase import quote
 from lp.services.propertycache import get_property_cache
 from lp.services.searchbuilder import any
 from lp.services.webapp.interfaces import (
@@ -220,15 +214,6 @@
         """Return the where clauses for visibility."""
         return []
 
-    def _getCandidateBranchesWith(self):
-        """Return WITH clauses defining candidate branches.
-
-        These are defined in terms of scope_branches which should be
-        separately calculated.
-        """
-        return [
-            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
@@ -388,14 +373,15 @@
             return DecoratedResultSet(resultset, pre_iter_hook=loader)
 
     def _scopedGetMergeProposals(self, statuses, eager_load=False):
-        scope_tables = [Branch] + self._tables.values()
-        scope_expressions = self._branch_filter_expressions
-        select = self.store.using(*scope_tables).find(
-            (Branch.id, Branch.information_type, Branch.ownerID),
-            *scope_expressions)
-        branches_query = select._get_select()
-        with_expr = [With("scope_branches", branches_query)
-            ] + self._getCandidateBranchesWith()
+        expressions = (
+            self._branch_filter_expressions
+            + self._getBranchVisibilityExpression())
+        with_expr = With(
+            "candidate_branches",
+            Select(
+                Branch.id,
+                tables=[Branch] + self._tables.values(),
+                where=And(*expressions) if expressions else True))
         expressions = [SQL("""
             source_branch IN (SELECT id FROM candidate_branches) AND
             target_branch IN (SELECT id FROM candidate_branches)""")]
@@ -763,20 +749,6 @@
         return [
             branch_class.information_type.is_in(PUBLIC_INFORMATION_TYPES)]
 
-    def _getCandidateBranchesWith(self):
-        """Return WITH clauses defining candidate branches.
-
-        These are defined in terms of scope_branches which should be
-        separately calculated.
-        """
-        # Anonymous users get public branches only.
-        return [
-            With("candidate_branches",
-                SQL("""select id from scope_branches
-                    where information_type in %s"""
-                % sqlvalues(PUBLIC_INFORMATION_TYPES)))
-            ]
-
 
 class VisibleBranchCollection(GenericBranchCollection):
     """A branch collection that has special logic for visibility."""
@@ -840,39 +812,6 @@
         return get_branch_privacy_filter(
             self._user, branch_class=branch_class)
 
-    def _getCandidateBranchesWith(self):
-        """Return WITH clauses defining candidate branches.
-
-        These are defined in terms of scope_branches which should be
-        separately calculated.
-        """
-        person = self._user
-        if person is None:
-            # Really an anonymous sitation
-            return [
-                With("candidate_branches",
-                    SQL("""select id from scope_branches
-                        where information_type in %s"""
-                    % sqlvalues(PUBLIC_INFORMATION_TYPES)))
-                ]
-        return [
-            With("teams", self.store.find(TeamParticipation.teamID,
-                TeamParticipation.personID == person.id)._get_select()),
-            With("private_branches", SQL("""
-                SELECT scope_branches.id FROM scope_branches WHERE
-                scope_branches.information_type in %s AND (
-                    (scope_branches.owner in (select team from teams) OR
-                     EXISTS(SELECT true from BranchSubscription, teams WHERE
-                         branchsubscription.branch = scope_branches.id AND
-                         branchsubscription.person = teams.team)))"""
-                % sqlvalues(PRIVATE_INFORMATION_TYPES))),
-            With("candidate_branches", SQL("""
-                (SELECT id FROM private_branches) UNION
-                (select id FROM scope_branches
-                WHERE information_type in %s)"""
-                % sqlvalues(PUBLIC_INFORMATION_TYPES)))
-            ]
-
     def visibleByUser(self, person):
         """See `IBranchCollection`."""
         if person == self._user: