launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Celso Providelo has proposed merging lp:~cprov/launchpad/private-branches-tips into lp:launchpad.

Requested reviews:
  William Grant (wgrant)

For more details, see:
https://code.launchpad.net/~cprov/launchpad/private-branches-tips/+merge/200530

Refactoring IDistribution.getBranchTips() to deal with branch privacy properly.
-- 
https://code.launchpad.net/~cprov/launchpad/private-branches-tips/+merge/200530
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2013-08-29 10:49:29 +0000
+++ lib/lp/registry/model/distribution.py	2014-01-06 13:28:59 +0000
@@ -10,7 +10,6 @@
     ]
 
 import itertools
-from operator import itemgetter
 
 from sqlobject import (
     BoolCol,
@@ -23,6 +22,7 @@
     Desc,
     Exists,
     Join,
+    LeftJoin,
     Max,
     Not,
     Or,
@@ -632,101 +632,61 @@
 
     def getBranchTips(self, user=None, since=None):
         """See `IDistribution`."""
-        # This, ignoring privacy issues, is what we want.
-        base_query = """
-        SELECT Branch.unique_name,
-               Branch.last_scanned_id,
-               SPBDS.name AS distro_series_name,
-               Branch.id,
-               Branch.information_type,
-               Branch.owner
-        FROM Branch
-        JOIN DistroSeries
-            ON Branch.distroseries = DistroSeries.id
-        LEFT OUTER JOIN SeriesSourcePackageBranch
-            ON Branch.id = SeriesSourcePackageBranch.branch
-        LEFT OUTER JOIN DistroSeries SPBDS
-            -- (SPDBS stands for Source Package Branch Distro Series)
-            ON SeriesSourcePackageBranch.distroseries = SPBDS.id
-        WHERE DistroSeries.distribution = %s
-        """ % sqlvalues(self.id)
+        from lp.code.model.branch import (
+            Branch, get_branch_privacy_filter)
+        from lp.code.model.seriessourcepackagebranch import (
+            SeriesSourcePackageBranch)
+
+        clauses = [
+            DistroSeries.distribution == self.id,
+            get_branch_privacy_filter(user),
+        ]
+
         if since is not None:
             # If "since" was provided, take into account.
-            base_query += (
-                '      AND branch.last_scanned > %s\n' % sqlvalues(since))
-        if user is None:
-            # Now we see just a touch of privacy concerns.
-            # If the current user is anonymous, they cannot see any private
-            # branches.
-            base_query += (
-                '      AND Branch.information_type in %s\n'
-                % sqlvalues(PUBLIC_INFORMATION_TYPES))
-        # We want to order the results, in part for easier grouping at the
-        # end.
-        base_query += 'ORDER BY unique_name, last_scanned_id'
-        if (user is None or
-            user.inTeam(getUtility(ILaunchpadCelebrities).admin)):
-            # Anonymous is already handled above; admins can see everything.
-            # In both cases, we can just use the query as it already stands.
-            query = base_query
-        else:
-            # Otherwise (an authenticated, non-admin user), we need to do some
-            # more sophisticated privacy dances.  Note that the one thing we
-            # are ignoring here is stacking.  See the discussion in comment 1
-            # of https://bugs.launchpad.net/launchpad/+bug/812335 . Often, we
-            # use unions for this kind of work.  The WITH statement can give
-            # us a similar approach with more flexibility. In both cases,
-            # we're essentially declaring that we have a better idea of a good
-            # high-level query plan than Postgres will.
-            query = """
-            WITH principals AS (
-                    SELECT team AS id
-                        FROM TeamParticipation
-                        WHERE TeamParticipation.person = %(user)s
-                    UNION
-                    SELECT %(user)s
-                ), all_branches AS (
-            %(base_query)s
-                ), private_branches AS (
-                    SELECT unique_name,
-                           last_scanned_id,
-                           distro_series_name,
-                           id,
-                           owner
-                    FROM all_branches
-                    WHERE information_type in %(private_branches)s
-                ), owned_branch_ids AS (
-                    SELECT private_branches.id
-                    FROM private_branches
-                    JOIN principals ON private_branches.owner = principals.id
-                ), subscribed_branch_ids AS (
-                    SELECT private_branches.id
-                    FROM private_branches
-                    JOIN BranchSubscription
-                        ON BranchSubscription.branch = private_branches.id
-                    JOIN principals
-                        ON BranchSubscription.person = principals.id
-                )
-            SELECT unique_name, last_scanned_id, distro_series_name
-            FROM all_branches
-            WHERE information_type in %(public_branches)s OR
-                  id IN (SELECT id FROM owned_branch_ids) OR
-                  id IN (SELECT id FROM subscribed_branch_ids)
-            """ % dict(
-                base_query=base_query,
-                user=quote(user.id),
-                private_branches=quote(PRIVATE_INFORMATION_TYPES),
-                public_branches=quote(PUBLIC_INFORMATION_TYPES))
-
-        data = Store.of(self).execute(query + ';')
-
+            clauses.append(Branch.last_scanned > since)
+
+        OfficialSeries = ClassAlias(DistroSeries)
+        branches = IStore(self).using(
+            Branch,
+            Join(DistroSeries,
+                 DistroSeries.id == Branch.distroseriesID),
+            LeftJoin(
+                SeriesSourcePackageBranch,
+                SeriesSourcePackageBranch.branchID == Branch.id),
+            LeftJoin(
+                OfficialSeries,
+                OfficialSeries.id == SeriesSourcePackageBranch.distroseriesID),
+        ).find(
+            # XXX cprov 20140106: Storm query specs with LeftJoin does not
+            # allow loading only specific fields ('Branch.unique_name',
+            # 'Branch.last_scanned_id', 'OfficialSeries.name') which could
+            # the load on DB.
+            (Branch, OfficialSeries),
+            And(*clauses)
+        ).order_by(
+            Branch.unique_name,
+            Branch.last_scanned_id
+        )
+
+        # Group/filter helpers.
+        def get_branch_key(item):
+            (branch, series) = item
+            return (branch.unique_name, branch.last_scanned_id)
+
+        def get_series_name(item):
+            (branch, series) = item
+            if series:
+                return series.name
+
+        # Group on location (unique_name) and revision (last_scanned_id).
         result = []
-        # Group on location (unique_name) and revision (last_scanned_id).
-        for key, group in itertools.groupby(data, itemgetter(0, 1)):
+        for key, group in itertools.groupby(branches, get_branch_key):
             result.append(list(key))
             # Pull out all the official series names and append them as a list
-            # to the end of the current record, removing Nones from the list.
-            result[-1].append(filter(None, map(itemgetter(2), group)))
+            # to the end of the current record.
+            result[-1].append(filter(None, map(get_series_name, group)))
+
         return result
 
     def getMirrorByName(self, name):