launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/blueprint-dependencies into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #707111 in Launchpad itself: "Blueprint dependency picker doesn't allow searching outside the target"
  https://bugs.launchpad.net/launchpad/+bug/707111

For more details, see:
https://code.launchpad.net/~thumper/launchpad/blueprint-dependencies/+merge/53191

Implement the all_deps and all_blocked attributes of blueprints using a recursive database query rather than a recursive algorithm.  Add some tests too.

Part of the work to allow any blueprint to be searched for when adding dependencies.
-- 
https://code.launchpad.net/~thumper/launchpad/blueprint-dependencies/+merge/53191
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/blueprint-dependencies into lp:launchpad.

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2011-03-08 00:40:50 +0000
+++ lib/lp/blueprints/model/specification.py	2011-03-14 03:19:02 +0000
@@ -6,6 +6,8 @@
 __metaclass__ = type
 __all__ = [
     'HasSpecificationsMixin',
+    'recursive_blocked_query',
+    'recursive_dependent_query',
     'Specification',
     'SpecificationSet',
     ]
@@ -84,6 +86,31 @@
 from lp.registry.interfaces.product import IProduct
 
 
+
+def recursive_blocked_query(spec):
+    return """
+        RECURSIVE blocked(id) AS (
+            SELECT %s
+        UNION
+            SELECT s.id
+            FROM specificationdependency sd, blocked b, specification s
+            WHERE sd.dependency = b.id
+            AND s.id = sd.specification
+        )""" % spec.id
+
+
+def recursive_dependent_query(spec):
+    return """
+        RECURSIVE dependencies(id) AS (
+            SELECT %s
+        UNION
+            SELECT s.id
+            FROM specificationdependency sd, dependencies d, specification s
+            WHERE sd.specification = d.id
+            AND s.id = sd.dependency
+        )""" % spec.id
+
+
 class Specification(SQLBase, BugLinkTargetMixin):
     """See ISpecification."""
 
@@ -609,42 +636,22 @@
                 SpecificationDependency.delete(deplink.id)
                 return deplink
 
-    def _find_all_deps(self, deps):
-        """This adds all dependencies of this spec (and their deps) to
-        deps.
-
-        The function is called recursively, as part of self.all_deps.
-        """
-        for dep in self.dependencies:
-            if dep not in deps:
-                deps.add(dep)
-                dep._find_all_deps(deps)
-
     @property
     def all_deps(self):
-        deps = set()
-        self._find_all_deps(deps)
-        return sorted(shortlist(deps),
-                    key=lambda s: (s.definition_status, s.priority, s.title))
-
-    def _find_all_blocked(self, blocked):
-        """This adds all blockers of this spec (and their blockers) to
-        blocked.
-
-        The function is called recursively, as part of self.all_blocked.
-        """
-        for blocker in self.blocked_specs:
-            if blocker not in blocked:
-                blocked.add(blocker)
-                blocker._find_all_blocked(blocked)
+        return Store.of(self).with_(
+            SQL(recursive_dependent_query(self))).find(
+            Specification,
+            Specification.id != self.id,
+            SQL('Specification.id in (select id from dependencies)'))
 
     @property
     def all_blocked(self):
         """See `ISpecification`."""
-        blocked = set()
-        self._find_all_blocked(blocked)
-        return sorted(blocked, key=lambda s: (s.definition_status,
-                                              s.priority, s.title))
+        return Store.of(self).with_(
+            SQL(recursive_blocked_query(self))).find(
+            Specification,
+            Specification.id != self.id,
+            SQL('Specification.id in (select id from blocked)'))
 
     # branches
     def getBranchLink(self, branch):

=== added directory 'lib/lp/blueprints/model/tests'
=== added file 'lib/lp/blueprints/model/tests/__init__.py'
=== added file 'lib/lp/blueprints/model/tests/test_specification.py'
--- lib/lp/blueprints/model/tests/test_specification.py	1970-01-01 00:00:00 +0000
+++ lib/lp/blueprints/model/tests/test_specification.py	2011-03-14 03:19:02 +0000
@@ -0,0 +1,73 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Unit tests for blueprints here."""
+
+__metaclass__ = type
+
+from testtools.matchers import Equals
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.testing import TestCaseWithFactory
+
+
+class TestSpecificationDependencies(TestCaseWithFactory):
+    """Test the methods for getting the dependencies for blueprints."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_no_deps(self):
+        blueprint = self.factory.makeBlueprint()
+        self.assertThat(list(blueprint.dependencies), Equals([]))
+        self.assertThat(list(blueprint.all_deps), Equals([]))
+        self.assertThat(list(blueprint.blocked_specs), Equals([]))
+        self.assertThat(list(blueprint.all_blocked), Equals([]))
+
+    def test_single_dependency(self):
+        do_first = self.factory.makeBlueprint()
+        do_next = self.factory.makeBlueprint()
+        do_next.createDependency(do_first)
+        self.assertThat(list(do_first.blocked_specs), Equals([do_next]))
+        self.assertThat(list(do_first.all_blocked), Equals([do_next]))
+        self.assertThat(list(do_next.dependencies), Equals([do_first]))
+        self.assertThat(list(do_next.all_deps), Equals([do_first]))
+
+    def test_linear_dependency(self):
+        do_first = self.factory.makeBlueprint()
+        do_next = self.factory.makeBlueprint()
+        do_next.createDependency(do_first)
+        do_last = self.factory.makeBlueprint()
+        do_last.createDependency(do_next)
+        self.assertThat(sorted(do_first.blocked_specs), Equals([do_next]))
+        self.assertThat(sorted(do_first.all_blocked), Equals([do_next, do_last]))
+        self.assertThat(sorted(do_last.dependencies), Equals([do_next]))
+        self.assertThat(sorted(do_last.all_deps), Equals([do_first, do_next]))
+
+    def test_diamond_dependency(self):
+        #             do_first
+        #            /        \
+        #    do_next_lhs    do_next_rhs
+        #            \        /
+        #             do_last
+        do_first = self.factory.makeBlueprint()
+        do_next_lhs = self.factory.makeBlueprint()
+        do_next_lhs.createDependency(do_first)
+        do_next_rhs = self.factory.makeBlueprint()
+        do_next_rhs.createDependency(do_first)
+        do_last = self.factory.makeBlueprint()
+        do_last.createDependency(do_next_lhs)
+        do_last.createDependency(do_next_rhs)
+        self.assertThat(
+            sorted(do_first.blocked_specs),
+            Equals([do_next_lhs, do_next_rhs]))
+        self.assertThat(
+            sorted(do_first.all_blocked),
+            Equals([do_next_lhs, do_next_rhs, do_last]))
+        self.assertThat(
+            sorted(do_last.dependencies),
+            Equals([do_next_lhs, do_next_rhs]))
+        self.assertThat(
+            sorted(do_last.all_deps),
+            Equals([do_first, do_next_lhs, do_next_rhs]))
+
+