launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/use-visibility-filter-spec-depends into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1095235 in Launchpad itself: "Bogus dependencies in Blueprint graph"
  https://bugs.launchpad.net/launchpad/+bug/1095235

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/use-visibility-filter-spec-depends/+merge/144626

Rewrite the query fragment returned by recursive_{blocked,dependent}_query to no longer insert the specification id by string interpolation, and to make use of get_specification_privacy_filter() so that specifications that the user can not view are not included in the dep tree. This also drops the spec argument to the function, since the callers will wrap the resultant string in SQL(), they can use params=() to set the specification id.

This also means that ISpecification.all_{blocked,deps} no longer require the sharing service to filter against visible artifacts, and the SQL queries perform the heavy lifting.

I've dropped recursive_dependent_query from __all__ since they are both pretty much internal-only functions, and I'd like to discourage their use if at all possible, but specificationdependency's vocab sadly makes use of recursive_blocked_query, so it stays. For now.
-- 
https://code.launchpad.net/~stevenk/launchpad/use-visibility-filter-spec-depends/+merge/144626
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/use-visibility-filter-spec-depends into lp:launchpad.

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2013-01-22 05:07:31 +0000
+++ lib/lp/blueprints/model/specification.py	2013-01-24 03:46:24 +0000
@@ -5,7 +5,6 @@
 __all__ = [
     'HasSpecificationsMixin',
     'recursive_blocked_query',
-    'recursive_dependent_query',
     'Specification',
     'SPECIFICATION_POLICY_ALLOWED_TYPES',
     'SPECIFICATION_POLICY_DEFAULT_TYPES',
@@ -101,6 +100,7 @@
 from lp.services.database.enumcol import EnumCol
 from lp.services.database.lpstorm import IStore
 from lp.services.database.sqlbase import (
+    convert_storm_clause_to_string,
     cursor,
     SQLBase,
     sqlvalues,
@@ -113,26 +113,34 @@
 from lp.services.webapp.interfaces import ILaunchBag
 
 
-def recursive_blocked_query(spec):
+def recursive_blocked_query(user):
+    from lp.blueprints.model.specificationsearch import (
+        get_specification_privacy_filter)
     return """
         RECURSIVE blocked(id) AS (
-            SELECT %s
+            SELECT ?
         UNION
             SELECT sd.specification
-            FROM specificationdependency sd, blocked b
-            WHERE sd.dependency = b.id
-        )""" % spec.id
-
-
-def recursive_dependent_query(spec):
+            FROM blocked b, specificationdependency sd
+            JOIN specification ON sd.specification = specification.id
+            WHERE sd.dependency = b.id AND (%s))""" % (
+                convert_storm_clause_to_string(
+                    *get_specification_privacy_filter(user)))
+
+
+def recursive_dependent_query(user):
+    from lp.blueprints.model.specificationsearch import (
+        get_specification_privacy_filter)
     return """
         RECURSIVE dependencies(id) AS (
-            SELECT %s
+            SELECT ?
         UNION
             SELECT sd.dependency
-            FROM specificationdependency sd, dependencies d
-            WHERE sd.specification = d.id
-        )""" % spec.id
+            FROM dependencies d, specificationdependency sd
+            JOIN specification ON sd.dependency = specification.id
+            WHERE sd.specification = d.id AND (%s))""" % (
+                convert_storm_clause_to_string(
+                    *get_specification_privacy_filter(user)))
 
 
 SPECIFICATION_POLICY_ALLOWED_TYPES = {
@@ -812,51 +820,21 @@
                 return deplink
 
     def all_deps(self, user=None):
-        public_clause = True
-        if user is None:
-            public_clause = (
-                Specification.information_type == InformationType.PUBLIC,
-                )
-
-        results = Store.of(self).with_(
-            SQL(recursive_dependent_query(self))).find(
+        return list(Store.of(self).with_(
+            SQL(recursive_dependent_query(user), params=(self.id,))).find(
             Specification,
             Specification.id != self.id,
-            SQL('Specification.id in (select id from dependencies)'),
-            public_clause,
-            ).order_by(Specification.name, Specification.id)
-
-        results = list(results)
-
-        if user:
-            service = getUtility(IService, 'sharing')
-            (ignore, ignore, results) = service.getVisibleArtifacts(
-                user, specifications=results)
-        return results
+            Specification.id.is_in(SQL('select id from dependencies')),
+            ).order_by(Specification.name, Specification.id))
 
     def all_blocked(self, user=None):
         """See `ISpecification`."""
-        public_clause = True
-        if user is None:
-            public_clause = (
-                Specification.information_type == InformationType.PUBLIC,
-                )
-
-        results = Store.of(self).with_(
-            SQL(recursive_blocked_query(self))).find(
+        return list(Store.of(self).with_(
+            SQL(recursive_blocked_query(user), params=(self.id,))).find(
             Specification,
             Specification.id != self.id,
-            SQL('Specification.id in (select id from blocked)'),
-            public_clause,
-            ).order_by(Specification.name, Specification.id)
-
-        results = list(results)
-
-        if user:
-            service = getUtility(IService, 'sharing')
-            (ignore, ignore, results) = service.getVisibleArtifacts(
-                user, specifications=results)
-        return results
+            Specification.id.is_in(SQL('select id from blocked')),
+            ).order_by(Specification.name, Specification.id))
 
     # branches
     def getBranchLink(self, branch):

=== modified file 'lib/lp/blueprints/vocabularies/specificationdependency.py'
--- lib/lp/blueprints/vocabularies/specificationdependency.py	2012-12-01 22:59:58 +0000
+++ lib/lp/blueprints/vocabularies/specificationdependency.py	2013-01-24 03:46:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The vocabularies relating to dependencies of specifications."""
@@ -24,7 +24,7 @@
     Specification,
     )
 from lp.registry.interfaces.pillar import IPillarNameSet
-from lp.services.database.sqlbase import quote
+from lp.services.database.stormexpr import fti_search
 from lp.services.webapp import (
     canonical_url,
     urlparse,
@@ -118,8 +118,10 @@
 
     def _exclude_blocked_query(self):
         """Return the select statement to exclude already blocked specs."""
-        return SQL("Specification.id not in (WITH %s select id from blocked)"
-                   % recursive_blocked_query(self.context))
+        user = getattr(getUtility(ILaunchBag), 'user', None)
+        return SQL(
+            "Specification.id not in (WITH %s select id from blocked)" % (
+                recursive_blocked_query(user)), params=(self.context.id,))
 
     def toTerm(self, obj):
         if obj.target == self.context.target:
@@ -182,7 +184,7 @@
 
         return Store.of(self.context).find(
             Specification,
-            SQL('Specification.fti @@ ftq(%s)' % quote(query)),
+            fti_search(Specification, query),
             self._exclude_blocked_query(),
             ).order_by(self._order_by())