Merge lp:~allenap/launchpad/subscribe-to-tag-bug-151129-2 into lp:launchpad

[Gavin Panella]

[r=adeuring,jcsackett][ui=none][bug=151129] Change the approach to finding structural subscriptions matching a bugtask.

This changes the way subscription filters are found. Instead of some
left joins and some array gymnastics there is a convenience class,
BugFilterSetBuilder, that combines sets of filters to figure out which
match. From that it can figure out which subscriptions match.

[j.c.sackett (jcsackett)]

Gavin--

This is a really nice branch! I like cutting out all of that logic into the utility.

I am a little weirded out by it living alone in models; the logic is a file in models should have a model in it--this one is just utilities for another model file. Since it doesn't have use outside of structuralsubscription.py, I think it might be better added to that file. I don't think that hurts the readability of that file, since the logic is still nicely contained in the class you created.

Additional comments are in the diff below.

> === modified file 'lib/lp/registry/model/structuralsubscription.py'
> --- lib/lp/registry/model/structuralsubscription.py 2010-11-18 16:11:43 +0000
> +++ lib/lp/registry/model/structuralsubscription.py 2010-11-18 16:11:51 +0000
> def getSubscriptionsForBugTask(self, bugtask, level):
> """See `IStructuralSubscriptionTarget`."""
> - origin = [
> - StructuralSubscription,
> - LeftJoin(
> - BugSubscriptionFilter,
> - BugSubscriptionFilter.structural_subscription_id == (
> - StructuralSubscription.id)),
> - LeftJoin(
> - BugSubscriptionFilterStatus,
> - BugSubscriptionFilterStatus.filter_id == (
> - BugSubscriptionFilter.id)),
> - LeftJoin(
> - BugSubscriptionFilterImportance,
> - BugSubscriptionFilterImportance.filter_id == (
> - BugSubscriptionFilter.id)),
> - ]
> -
> - # An ARRAY[] expression for the given bug's tags.
> - tags_array = "ARRAY[%s]::TEXT[]" % ",".join(
> - quote(tag) for tag in bugtask.bug.tags)
> -
> - # The tags a subscription requests for inclusion.
> - tags_include_expr = (
> - "SELECT tag FROM BugSubscriptionFilterTag "
> - "WHERE filter = BugSubscriptionFilter.id AND include")
> - tags_include_array = "ARRAY(%s)" % tags_include_expr
> - tags_include_is_empty = SQL(
> - "ARRAY[]::TEXT[] = %s" % tags_include_array)
> -
> - # The tags a subscription requests for exclusion.
> - tags_exclude_expr = (
> - "SELECT tag FROM BugSubscriptionFilterTag "
> - "WHERE filter = BugSubscriptionFilter.id AND NOT include")
> - tags_exclude_array = "ARRAY(%s)" % tags_exclude_expr
> - tags_exclude_is_empty = SQL(
> - "ARRAY[]::TEXT[] = %s" % tags_exclude_array)
> -
> - # Choose the correct expression depending on the find_all_tags flag.
> - def tags_find_all_combinator(find_all_expr, find_any_expr):
> - return SQL(
> - "CASE WHEN BugSubscriptionFilter.find_all_tags "
> - "THEN (%s) ELSE (%s) END" % (find_all_expr, find_any_expr))
> -
> - if len(bugtask.bug.tags) == 0:
> - tag_conditions = [
> - BugSubscriptionFilter.include_any_tags == False,
> - # The subscription's required tags must be an empty set.
> - tags_include_is_empty,
> - # The subscription's excluded tags can be anything so no
> - # condition...

review: Needs Information (code*)

[Gavin Panella (allenap)]

Thanks for the excellent review :)

> I am a little weirded out by it living alone in models; the logic is a file in
> models should have a model in it--this one is just utilities for another model
> file. Since it doesn't have use outside of structuralsubscription.py, I think
> it might be better added to that file. I don't think that hurts the
> readability of that file, since the logic is still nicely contained in the
> class you created.

Yeah, fair point. I moved it out because it helped me concentrate
while working on it, but it was a lot messier then. I've moved it
back.

...
> > + # Set up common filter conditions.
> > + if len(self.tags) == 0:
> > + self.filter_conditions = And(
> > + BugSubscriptionFilter.include_any_tags == False,
> > + self.base_conditions)
> > + else:
> > + self.filter_conditions = And(
> > + BugSubscriptionFilter.exclude_any_tags == False,
> > + self.base_conditions)
> > +
>
> A quick comment here explaining the logic of setting up exclude_any vs
> include_any would be good.

Done.

...
> > + def _filters_matching_x(self, join, where_condition, **extra):
> > + # The expressions returned by this function are used in set (union,
> > + # intersect, except) operations at the *filter* level. However, the
> > + # interesting result of these set operations is the structural
> > + # subscription, hence both columns are included in the expressions
> > + # generated. Since a structural subscription can have zero or more
> > + # filters, and a filter can never be associated with more than one
> > + # subscription, the set operations are unaffected.
> > + return Select(
> > + columns=(
> > + # Alias this column so it can be selected in
> > + # subscriptions_matching.
> > + Alias(
> > + BugSubscriptionFilter.structural_subscription_id,
> > + "structural_subscription_id"),
> > + BugSubscriptionFilter.id),
> > + tables=(
> > + StructuralSubscription, BugSubscriptionFilter, join),
> > + where=And(
> > + BugSubscriptionFilter.structural_subscription_id == (
> > + StructuralSubscription.id),
> > + self.filter_conditions,
> > + where_condition),
> > + **extra)
> > +
>
> I would prefer some sort of docstring for this in place of in addition to the
> comment block at the top.

Done.

...
> > + def _filters_matching_all_x_tags(self, where_condition):
> > + tags_array = "ARRAY[%s]::TEXT[]" % ",".join(
> > + quote(tag) for tag in self.tags)
> > + return self._filters_matching_x(
> > + BugSubscriptionFilterTag,
> > + And(
> > + BugSubscriptionFilterTag.filter_id == (
> > + BugSubscriptionFilter.id),
> > + BugSubscriptionFilter.find_all_tags,
> > + self.filter_conditions,
> > + where_condition),
> > + group_by=(
> > + ...

[Abel Deuring (adeuring)]

A very nice branch and a very nice review!

Just one more nitpick: Gavin, could you add a doc string to BugFilterSetBiulder.__init__(), and especially :param ...: lines? What bugtask and level means is obvious to me, but I had to poke around a bit to figure out where join_codition comes from.

review: Approve (code)

[j.c.sackett (jcsackett)]

Gavin--

I like those changes. Thanks!

review: Approve (code*)

=== modified file 'lib/lp/registry/model/structuralsubscription.py'
--- lib/lp/registry/model/structuralsubscription.py	2010-11-24 09:42:16 +0000
+++ lib/lp/registry/model/structuralsubscription.py	2010-11-24 09:42:18 +0000
@@ -2,15 +2,26 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
-__all__ = ['StructuralSubscription',
-           'StructuralSubscriptionTargetMixin']
+__all__ = [
+    'StructuralSubscription',
+    'StructuralSubscriptionTargetMixin',
+    ]
 
 from sqlobject import ForeignKey
 from storm.expr import (
+    Alias,
     And,
+    CompoundOper,
+    Except,
+    In,
+    Intersect,
     LeftJoin,
+    NamedFunc,
+    Not,
     Or,
+    Select,
     SQL,
+    Union,
     )
 from storm.store import Store
 from zope.component import (
@@ -35,6 +46,7 @@
 from lp.bugs.model.bugsubscriptionfilterstatus import (
     BugSubscriptionFilterStatus,
     )
+from lp.bugs.model.bugsubscriptionfiltertag import BugSubscriptionFilterTag
 from lp.registry.enum import BugNotificationLevel
 from lp.registry.errors import (
     DeleteSubscriptionError,
@@ -472,93 +484,242 @@
 
     def getSubscriptionsForBugTask(self, bugtask, level):
         """See `IStructuralSubscriptionTarget`."""
-        origin = [
-            StructuralSubscription,
-            LeftJoin(
-                BugSubscriptionFilter,
+        set_builder = BugFilterSetBuilder(
+            bugtask, level, self.__helper.join)
+        return Store.of(self.__helper.pillar).find(
+            StructuralSubscription, In(
+                StructuralSubscription.id,
+                set_builder.subscriptions))
+
+
+class ArrayAgg(NamedFunc):
+    __slots__ = ()
+    name = "ARRAY_AGG"
+
+
+class ArrayContains(CompoundOper):
+    __slots__ = ()
+    oper = "@>"
+
+
+class BugFilterSetBuilder:
+    """A convenience class to build queries for getSubscriptionsForBugTask."""
+
+    def __init__(self, bugtask, level, join_condition):
+        """Initialize a new set builder for bug filters.
+
+        :param bugtask: The `IBugTask` to match against.
+        :param level: A member of `BugNotificationLevel`.
+        :param join_condition: A condition for selecting structural
+            subscriptions. Generally this should limit the subscriptions to a
+            particular target (i.e. project or distribution).
+        """
+        self.status = bugtask.status
+        self.importance = bugtask.importance
+        # The list() gets around some weirdness with security proxies; Storm
+        # does not know how to compile an expression with a proxied list.
+        self.tags = list(bugtask.bug.tags)
+        # Set up common conditions.
+        self.base_conditions = And(
+            StructuralSubscription.bug_notification_level >= level,
+            join_condition)
+        # Set up common filter conditions.
+        if len(self.tags) == 0:
+            self.filter_conditions = And(
+                # When the bug has no tags, filters with include_any_tags set
+                # can never match.
+                Not(BugSubscriptionFilter.include_any_tags),
+                self.base_conditions)
+        else:
+            self.filter_conditions = And(
+                # When the bug has tags, filters with exclude_any_tags set can
+                # never match.
+                Not(BugSubscriptionFilter.exclude_any_tags),
+                self.base_conditions)
+
+    @property
+    def subscriptions_without_filters(self):
+        """Subscriptions without filters."""
+        return Select(
+            StructuralSubscription.id,
+            tables=(
+                StructuralSubscription,
+                LeftJoin(
+                    BugSubscriptionFilter,
+                    BugSubscriptionFilter.structural_subscription_id == (
+                        StructuralSubscription.id))),
+            where=And(
+                BugSubscriptionFilter.id == None,
+                self.base_conditions))
+
+    def _filters_matching_x(self, join, where_condition, **extra):
+        """Return an expression yielding `(subscription_id, filter_id)` rows.
+
+        The expressions returned by this function are used in set (union,
+        intersect, except) operations at the *filter* level. However, the
+        interesting result of these set operations is the structural
+        subscription, hence both columns are included in the expressions
+        generated. Since a structural subscription can have zero or more
+        filters, and a filter can never be associated with more than one
+        subscription, the set operations are unaffected.
+        """
+        return Select(
+            columns=(
+                # Alias this column so it can be selected in
+                # subscriptions_matching.
+                Alias(
+                    BugSubscriptionFilter.structural_subscription_id,
+                    "structural_subscription_id"),
+                BugSubscriptionFilter.id),
+            tables=(
+                StructuralSubscription, BugSubscriptionFilter, join),
+            where=And(
                 BugSubscriptionFilter.structural_subscription_id == (
-                    StructuralSubscription.id)),
-            LeftJoin(
-                BugSubscriptionFilterStatus,
-                BugSubscriptionFilterStatus.filter_id == (
-                    BugSubscriptionFilter.id)),
-            LeftJoin(
-                BugSubscriptionFilterImportance,
-                BugSubscriptionFilterImportance.filter_id == (
-                    BugSubscriptionFilter.id)),
-            ]
-
-        # An ARRAY[] expression for the given bug's tags.
+                    StructuralSubscription.id),
+                self.filter_conditions,
+                where_condition),
+            **extra)
+
+    @property
+    def filters_matching_status(self):
+        """Filters with the given bugtask's status."""
+        join = LeftJoin(
+            BugSubscriptionFilterStatus,
+            BugSubscriptionFilterStatus.filter_id == (
+                BugSubscriptionFilter.id))
+        condition = Or(
+            BugSubscriptionFilterStatus.id == None,
+            BugSubscriptionFilterStatus.status == self.status)
+        return self._filters_matching_x(join, condition)
+
+    @property
+    def filters_matching_importance(self):
+        """Filters with the given bugtask's importance."""
+        join = LeftJoin(
+            BugSubscriptionFilterImportance,
+            BugSubscriptionFilterImportance.filter_id == (
+                BugSubscriptionFilter.id))
+        condition = Or(
+            BugSubscriptionFilterImportance.id == None,
+            BugSubscriptionFilterImportance.importance == self.importance)
+        return self._filters_matching_x(join, condition)
+
+    @property
+    def filters_without_include_tags(self):
+        """Filters with no tags required."""
+        join = LeftJoin(
+            BugSubscriptionFilterTag,
+            And(BugSubscriptionFilterTag.filter_id == (
+                    BugSubscriptionFilter.id),
+                BugSubscriptionFilterTag.include))
+        return self._filters_matching_x(
+            join, BugSubscriptionFilterTag.id == None)
+
+    @property
+    def filters_matching_any_include_tags(self):
+        """Filters including any of the bug's tags."""
+        condition = And(
+            BugSubscriptionFilterTag.filter_id == (
+                BugSubscriptionFilter.id),
+            BugSubscriptionFilterTag.include,
+            Not(BugSubscriptionFilter.find_all_tags),
+            In(BugSubscriptionFilterTag.tag, self.tags))
+        return self._filters_matching_x(
+            BugSubscriptionFilterTag, condition)
+
+    @property
+    def filters_matching_any_exclude_tags(self):
+        """Filters excluding any of the bug's tags."""
+        condition = And(
+            BugSubscriptionFilterTag.filter_id == (
+                BugSubscriptionFilter.id),
+            Not(BugSubscriptionFilterTag.include),
+            Not(BugSubscriptionFilter.find_all_tags),
+            In(BugSubscriptionFilterTag.tag, self.tags))
+        return self._filters_matching_x(
+            BugSubscriptionFilterTag, condition)
+
+    def _filters_matching_all_x_tags(self, where_condition):
+        """Return an expression yielding `(subscription_id, filter_id)` rows.
+
+        This joins to `BugSubscriptionFilterTag` and calls up to
+        `_filters_matching_x`, and groups by filter. Conditions are added to
+        ensure that all rows in each group are a subset of the bug's tags.
+        """
         tags_array = "ARRAY[%s]::TEXT[]" % ",".join(
-            quote(tag) for tag in bugtask.bug.tags)
-
-        # The tags a subscription requests for inclusion.
-        tags_include_expr = (
-            "SELECT tag FROM BugSubscriptionFilterTag "
-            "WHERE filter = BugSubscriptionFilter.id AND include")
-        tags_include_array = "ARRAY(%s)" % tags_include_expr
-        tags_include_is_empty = SQL(
-            "ARRAY[]::TEXT[] = %s" % tags_include_array)
-
-        # The tags a subscription requests for exclusion.
-        tags_exclude_expr = (
-            "SELECT tag FROM BugSubscriptionFilterTag "
-            "WHERE filter = BugSubscriptionFilter.id AND NOT include")
-        tags_exclude_array = "ARRAY(%s)" % tags_exclude_expr
-        tags_exclude_is_empty = SQL(
-            "ARRAY[]::TEXT[] = %s" % tags_exclude_array)
-
-        # Choose the correct expression depending on the find_all_tags flag.
-        def tags_find_all_combinator(find_all_expr, find_any_expr):
-            return SQL(
-                "CASE WHEN BugSubscriptionFilter.find_all_tags "
-                "THEN (%s) ELSE (%s) END" % (find_all_expr, find_any_expr))
-
-        if len(bugtask.bug.tags) == 0:
-            tag_conditions = [
-                BugSubscriptionFilter.include_any_tags == False,
-                # The subscription's required tags must be an empty set.
-                tags_include_is_empty,
-                # The subscription's excluded tags can be anything so no
-                # condition is needed.
-                ]
+            quote(tag) for tag in self.tags)
+        return self._filters_matching_x(
+            BugSubscriptionFilterTag,
+            And(
+                BugSubscriptionFilterTag.filter_id == (
+                    BugSubscriptionFilter.id),
+                BugSubscriptionFilter.find_all_tags,
+                self.filter_conditions,
+                where_condition),
+            group_by=(
+                BugSubscriptionFilter.structural_subscription_id,
+                BugSubscriptionFilter.id),
+            having=ArrayContains(
+                SQL(tags_array), ArrayAgg(
+                    BugSubscriptionFilterTag.tag)))
+
+    @property
+    def filters_matching_all_include_tags(self):
+        """Filters including the bug's tags."""
+        return self._filters_matching_all_x_tags(
+            BugSubscriptionFilterTag.include)
+
+    @property
+    def filters_matching_all_exclude_tags(self):
+        """Filters excluding the bug's tags."""
+        return self._filters_matching_all_x_tags(
+            Not(BugSubscriptionFilterTag.include))
+
+    @property
+    def filters_matching_include_tags(self):
+        """Filters with tag filters including the bug."""
+        return Union(
+            self.filters_matching_any_include_tags,
+            self.filters_matching_all_include_tags)
+
+    @property
+    def filters_matching_exclude_tags(self):
+        """Filters with tag filters excluding the bug."""
+        return Union(
+            self.filters_matching_any_exclude_tags,
+            self.filters_matching_all_exclude_tags)
+
+    @property
+    def filters_matching_tags(self):
+        """Filters with tag filters matching the bug."""
+        if len(self.tags) == 0:
+            # The filter's required tags must be an empty set. The filter's
+            # excluded tags can be anything so no condition is needed.
+            return self.filters_without_include_tags
         else:
-            tag_conditions = [
-                BugSubscriptionFilter.exclude_any_tags == False,
-                # The bug's tags must contain the subscription's required tags
-                # if find_all_tags is set, else there must be an intersection.
-                Or(tags_include_is_empty,
-                   tags_find_all_combinator(
-                        "%s @> %s" % (tags_array, tags_include_array),
-                        "%s && %s" % (tags_array, tags_include_array))),
-                # The bug's tags must not contain the subscription's excluded
-                # tags if find_all_tags is set, else there must not be an
-                # intersection.
-                Or(tags_exclude_is_empty,
-                   tags_find_all_combinator(
-                        "NOT (%s @> %s)" % (tags_array, tags_exclude_array),
-                        "NOT (%s && %s)" % (tags_array, tags_exclude_array))),
-                ]
-
-        conditions = [
-            StructuralSubscription.bug_notification_level >= level,
-            Or(
-                # There's no filter or ...
-                BugSubscriptionFilter.id == None,
-                # There is a filter and ...
-                And(
-                    # There's no status filter, or there is a status filter
-                    # and and it matches.
-                    Or(BugSubscriptionFilterStatus.id == None,
-                       BugSubscriptionFilterStatus.status == bugtask.status),
-                    # There's no importance filter, or there is an importance
-                    # filter and it matches.
-                    Or(BugSubscriptionFilterImportance.id == None,
-                       BugSubscriptionFilterImportance.importance == (
-                            bugtask.importance)),
-                    # Any number of conditions relating to tags.
-                    *tag_conditions)),
-            ]
-
-        return Store.of(self.__helper.pillar).using(*origin).find(
-            StructuralSubscription, self.__helper.join, *conditions)
+            return Except(
+                Union(self.filters_without_include_tags,
+                      self.filters_matching_include_tags),
+                self.filters_matching_exclude_tags)
+
+    @property
+    def filters_matching(self):
+        """Filters matching the bug."""
+        return Intersect(
+            self.filters_matching_status,
+            self.filters_matching_importance,
+            self.filters_matching_tags)
+
+    @property
+    def subscriptions_with_matching_filters(self):
+        """Subscriptions with one or more filters matching the bug."""
+        return Select(
+            # I don't know of a more Storm-like way of doing this.
+            SQL("filters_matching.structural_subscription_id"),
+            tables=Alias(self.filters_matching, "filters_matching"))
+
+    @property
+    def subscriptions(self):
+        return Union(
+            self.subscriptions_without_filters,
+            self.subscriptions_with_matching_filters)

=== modified file 'lib/lp/registry/tests/test_structuralsubscriptiontarget.py'
--- lib/lp/registry/tests/test_structuralsubscriptiontarget.py	2010-11-24 09:42:16 +0000
+++ lib/lp/registry/tests/test_structuralsubscriptiontarget.py	2010-11-24 09:42:18 +0000
@@ -67,7 +67,7 @@
 
 
 class RestrictedStructuralSubscription(StructuralSubscriptionTestBase):
-    # Tests suitable for a target that restricts structural subscriptions.
+    """Tests suitable for a target that restricts structural subscriptions."""
 
     def test_target_implements_structural_subscription_target(self):
         self.assertTrue(verifyObject(IStructuralSubscriptionTarget,
@@ -128,8 +128,10 @@
 
 
 class UnrestrictedStructuralSubscription(RestrictedStructuralSubscription):
-    # Tests suitable for a target that does not restrict structural
-    # subscriptions.
+    """
+    Tests suitable for a target that does not restrict structural
+    subscriptions.
+    """
 
     def test_structural_subscription_by_ordinary_user(self):
         # ordinary users can subscribe themselves
@@ -409,6 +411,35 @@
         self.bug.tags = ["bar", "baz"]
         self.assertSubscriptions([self.subscription])
 
+    def test_getSubscriptionsForBugTask_any_filter_is_a_match(self):
+        # If a subscription has multiple filters, the subscription is selected
+        # when any filter is found to match. Put another way, the filters are
+        # ORed together.
+        subscription_filter1 = self.subscription.newBugFilter()
+        subscription_filter1.statuses = [BugTaskStatus.CONFIRMED]
+        subscription_filter2 = self.subscription.newBugFilter()
+        subscription_filter2.tags = [u"foo"]
+
+        # With the filter the subscription is not found.
+        self.assertSubscriptions([])
+
+        # If the bugtask is adjusted to match the criteria of the first filter
+        # but not those of the second, the subscription is found.
+        self.bugtask.transitionToStatus(
+            BugTaskStatus.CONFIRMED, self.ordinary_subscriber)
+        self.assertSubscriptions([self.subscription])
+
+        # If the filter is adjusted to also match the criteria of the second
+        # filter, the subscription is still found.
+        self.bugtask.bug.tags = [u"foo"]
+        self.assertSubscriptions([self.subscription])
+
+        # If the bugtask is adjusted to no longer match the criteria of the
+        # first filter, the subscription is still found.
+        self.bugtask.transitionToStatus(
+            BugTaskStatus.INPROGRESS, self.ordinary_subscriber)
+        self.assertSubscriptions([self.subscription])
+
 
 class TestStructuralSubscriptionForDistro(
     RestrictedStructuralSubscription, TestCaseWithFactory):