Merge lp:~adeuring/launchpad/bug-594247 into lp:launchpad

[Abel Deuring]

[r=allenap,gmb][ui=none][bug=594247] use JOIN and LEFT JOIN to link tables needed to search for bugtask releated to a structural subscriber; refactoring of BugTaskSet.search() and BugTaskSet.serachBugIds(); more tests for BugTaskSet.search()

This branch fixes bug 594247: searchTasks with structural_subscriber
times out regularly.

The fix uses stub's suggestion from a comment in the bug report
to (LEFT) JOIN the tables Product and StructuralSubscription and
to define the filter clause "StructuralSubscriber=somebody"
directly in the main query.

This required some larger modification of the method
BugTaskSet.buildQuery(): Up to now, it returned four variables,
query, clauseTables, orderby_arg, decorator. query contains a string
with the WHERE clause, clauseTables contains a list of tables
used in the WHERE clause.

Simply adding Storm Join instances for

  LEFT OUTER JOIN Product ON BugTask.product=Product.id

and

  JOIN StructuralSubscription ON (some_longer_expression)

to clauseTables has at least one problem: The method BugTaskSet.search()
optionally joins some tables, including Product, in order to pre-load
some objects that will be needed later to process a request.

So it could happen that the table Product is JOINed twice: The first time
in clauseTables because it is needed in the WHERE expression for
structural subscription, the second time to pre-load Storm Product
instances.

Hence I did not add the Storm Join instances to clauseTables; instead,
BugTaskSet.buildQuery() now returns another parameter join_tables,
which contains a sequence of tuples (storm_table, join_expression).

A new method BugTaskSet.buildOrigin() now processes join_expression and
clauseTables together with a sequence of Join instances needed to
pre-load Product, Bug, SourcePackageName instances, so that each
table appears exactly once in the WHERE clause of the SQL query.

The new way to find bugtasks related to a structural subscription
can return bug tasks twice, for example, if somebody has
a structural subscription to a distribution and to a distro source
package. So BugTaskSet.buildQuery() now returns one more parameter,
has_duplicate_results.

BugTaskSet.buildQuery() was called by BugTaskSet.search() and
by BugTaskSet.searchBugIds(). Processing the data returned by
buildQuery() became a bit more complicated, and BugTaskSet.search()
could already create two variants of the SQL query: one to retrieve
only BugTask instances and another to retrieve related Product,
Bug, SourcePackageName instances. We can consider searchBugIds()
to be variant of search() which just defines another type of data to
retrieve, so the SQL query is now built in a new method _search()
which is called by search() and by searchBugIds().

This required another tweak: BugTaskSet.buildQuery() returns a
decorator for the result set whichs avoids later SQL queries
ensuring that the current user is allowed to view a bug. (see
lp.bugs.model.bugtask.get_bug_privacy_filter_with_decorator())

This decorator assumes that the Storm query returns BugTask
instances, which is not the case for BugTaskSet.searchBugIds().
So BugTaskSet._search() has an optional parameter "undecorated".
If it is True, _search() returns a plain Storm result set,
otherwise, it returns a DecoratedResultSet.

Treatment of queries which may have duplicate result rows
("if has_duplicate_result_rows:" in _search()): We cannot simply
call resultset.config(distinct=True) because we may get two rows
for one bugtask if somebody is subscribed to a distribution and
to a distro source package and if we preload SourcePackageName.

Using a DISTINCT ON BugTask.id query would require sorting by
BugTask.id, so I simply used a sub-query.

The part of BugTaskSet.search() (now in _search()) which handles
queries with more that one BugTaskSearchParams instance now
preloads/prejoins Bug, Product etc only if _noprejoins is False.

Finally, I added some more tests to test_bugtask_search.py

no lint.

test: ./bin/test -vvt test_bugtask_search

It might make sense to change BugTaskSet.getAssignedMilestonesFromSearch()
in a follow-up brnach so that it calls _search() directly,
instead of calling search(), extracting milestone IDs
from the result set and running another SQL query to retrieve
these milestones.

[Graham Binns (gmb)]

Hi Abel,

As we discussed on IRC, I think you should get a review from someone more SQL / Storm savvy than I, since my brain seems to just skip off those parts of the code.

I'm happy with the rest of the diff, with just two comments:

> 94 + # Circular.

Please expand this to something less terse ;). ("Prevent circular
import problems." or something similar.)

> 319 + # Circular.

Here too.

review: Approve (code)

[Gavin Panella (allenap)]

Cool :) Lots of comments, but none of them are very important.

[1]

+ Or(BugTask.sourcepackagenameID ==
+ StructuralSubscription.sourcepackagenameID,
+ StructuralSubscription.sourcepackagename == None)))

Indentation is a little confusing there.

[2]

+ join_clause = (
+ BugTask.productID == StructuralSubscription.productID)
+ join_clause = Or(
+ join_clause,
+ (BugTask.productseriesID ==
+ StructuralSubscription.productseriesID))
+ # Circular.
+ from lp.registry.model.product import Product
+ join_clause = Or(
+ join_clause,
+ And(Product.projectID == StructuralSubscription.projectID,
+ BugTask.product == Product.id))
+ join_clause = Or(
+ join_clause,
+ And((BugTask.distributionID ==
+ StructuralSubscription.distributionID),
+ Or(BugTask.sourcepackagenameID ==
+ StructuralSubscription.sourcepackagenameID,
+ StructuralSubscription.sourcepackagename == None)))
+ join_clause = Or(
+ join_clause,
+ (BugTask.distroseriesID ==
+ StructuralSubscription.distroseriesID))
+ join_clause = Or(
+ join_clause,
+ BugTask.milestoneID == StructuralSubscription.milestoneID)

I had trouble reading this, so I refactored it to use more descriptive
names. If you like what I've done then I think it would be a good
change to make:

    ssub_match_product = (
        BugTask.productID ==
        StructuralSubscription.productID)
    ssub_match_productseries = (
        BugTask.productseriesID ==
        StructuralSubscription.productseriesID)
    ssub_match_project = And(
        Product.projectID ==
        StructuralSubscription.projectID,
        BugTask.product == Product.id)
    ssub_match_distribution = (
        BugTask.distributionID ==
        StructuralSubscription.distributionID)
    ssub_match_sourcepackagename = (
        BugTask.sourcepackagenameID ==
        StructuralSubscription.sourcepackagenameID)
    ssub_match_null_sourcepackagename = (
        StructuralSubscription.sourcepackagename == None)
    ssub_match_distribution_with_optional_package = And(
        ssub_match_distribution, Or(
            ssub_match_sourcepackagename,
            ssub_match_null_sourcepackagename))
    ssub_match_distribution_series = (
        BugTask.distroseriesID ==
        StructuralSubscription.distroseriesID)
    ssub_match_milestone = (
        BugTask.milestoneID ==
        StructuralSubscription.milestoneID)

    join_clause = Or(
        ssub_match_product,
        ssub_match_productseries,
        ssub_match_project,
        ssub_match_distribution_with_optional_package,
        ssub_match_distribution_series,
        ssub_match_milestone)

[3]

+ query, clauseTables, orderby_arg, decorator, join_tables,

Just because everything else seems to not be camel-case any more,
consider doing s/clauseTables/clause_...

review: Approve (code)

[Abel Deuring (adeuring)]

Hi Gavin,

thanks for your thorough review!

On 12.11.2010 17:29, Gavin Panella wrote:
> Review: Approve code
> Cool :) Lots of comments, but none of them are very important.
>
>
> [1]
>
> + Or(BugTask.sourcepackagenameID ==
> + StructuralSubscription.sourcepackagenameID,
> + StructuralSubscription.sourcepackagename == None)))
>
> Indentation is a little confusing there.

yeah... But your proposal below to make the huge join clause more
readable fixes it already :)

>
>
> [2]
>
> + join_clause = (
> + BugTask.productID == StructuralSubscription.productID)
> + join_clause = Or(
> + join_clause,
> + (BugTask.productseriesID ==
> + StructuralSubscription.productseriesID))
> + # Circular.
> + from lp.registry.model.product import Product
> + join_clause = Or(
> + join_clause,
> + And(Product.projectID == StructuralSubscription.projectID,
> + BugTask.product == Product.id))
> + join_clause = Or(
> + join_clause,
> + And((BugTask.distributionID ==
> + StructuralSubscription.distributionID),
> + Or(BugTask.sourcepackagenameID ==
> + StructuralSubscription.sourcepackagenameID,
> + StructuralSubscription.sourcepackagename == None)))
> + join_clause = Or(
> + join_clause,
> + (BugTask.distroseriesID ==
> + StructuralSubscription.distroseriesID))
> + join_clause = Or(
> + join_clause,
> + BugTask.milestoneID == StructuralSubscription.milestoneID)
>
> I had trouble reading this, so I refactored it to use more descriptive
> names. If you like what I've done then I think it would be a good
> change to make:
>
> ssub_match_product = (
> BugTask.productID ==
> StructuralSubscription.productID)
> ssub_match_productseries = (
> BugTask.productseriesID ==
> StructuralSubscription.productseriesID)
> ssub_match_project = And(
> Product.projectID ==
> StructuralSubscription.projectID,
> BugTask.product == Product.id)
> ssub_match_distribution = (
> BugTask.distributionID ==
> StructuralSubscription.distributionID)
> ssub_match_sourcepackagename = (
> BugTask.sourcepackagenameID ==
> StructuralSubscription.sourcepackagenameID)
> ssub_match_null_sourcepackagename = (
> StructuralSubscription.sourcepackagename == None)
> ssub_match_distribution_with_optional_package = And(
> ssub_match_distribution, Or(
> ssub_match_sourcepackagename,
> ssub_match_null_sourcepackagename))
> ssub_match_distribution_series = (
> BugTask.distroseriesID ==
> StructuralSubscription.distroseriesID)
> ssub_match_milestone = (
> BugTask.milestoneID ==
> StructuralSubscription.milestoneID)
>
> join_clause = Or(
> ssub_match_product,
> ...

[Robert Collins (lifeless)]

"
This decorator assumes that the Storm query returns BugTask
instances, which is not the case for BugTaskSet.searchBugIds().
So BugTaskSet._search() has an optional parameter "undecorated".
If it is True, _search() returns a plain Storm result set,
otherwise, it returns a DecoratedResultSet."

I have a thought here: the decorator is meant to be opaque. So why not
just return an appropriate one for the query being constructed?

I guess I'm saying that 'undecorated' is the what, not the why, and
some other approach might be clearer.

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2010-11-09 08:43:34 +0000
+++ lib/lp/bugs/model/bugtask.py	2010-11-17 13:53:59 +0000
@@ -35,11 +35,12 @@
 from storm.expr import (
     Alias,
     And,
-    AutoTables,
     Desc,
+    In,
     Join,
     LeftJoin,
     Or,
+    Select,
     SQL,
     )
 from storm.store import (
@@ -160,6 +161,7 @@
     )
 from lp.registry.model.pillar import pillar_sort_key
 from lp.registry.model.sourcepackagename import SourcePackageName
+from lp.registry.model.structuralsubscription import StructuralSubscription
 from lp.services.propertycache import get_property_cache
 from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.model.publishing import SourcePackagePublishingHistory
@@ -1606,7 +1608,9 @@
         from lp.bugs.model.bug import Bug
         extra_clauses = ['Bug.id = BugTask.bug']
         clauseTables = [BugTask, Bug]
+        join_tables = []
         decorators = []
+        has_duplicate_results = False
 
         # These arguments can be processed in a loop without any other
         # special handling.
@@ -1662,7 +1666,7 @@
             extra_clauses.append("BugTask.milestone %s" % where_cond)
 
         if params.project:
-            # Circular.
+            # Prevent circular import problems.
             from lp.registry.model.product import Product
             clauseTables.append(Product)
             extra_clauses.append("BugTask.product = Product.id")
@@ -1713,47 +1717,54 @@
                     sqlvalues(personid=params.subscriber.id))
 
         if params.structural_subscriber is not None:
-            structural_subscriber_clause = ("""BugTask.id IN (
-                SELECT BugTask.id FROM BugTask, StructuralSubscription
-                WHERE BugTask.product = StructuralSubscription.product
-                  AND StructuralSubscription.subscriber = %(personid)s
-                UNION ALL
-                SELECT BugTask.id FROM BugTask, StructuralSubscription
-                WHERE
-                  BugTask.distribution = StructuralSubscription.distribution
-                  AND BugTask.sourcepackagename =
-                      StructuralSubscription.sourcepackagename
-                  AND StructuralSubscription.subscriber = %(personid)s
-                UNION ALL
-                SELECT BugTask.id FROM BugTask, StructuralSubscription
-                WHERE
-                  BugTask.distroseries = StructuralSubscription.distroseries
-                  AND StructuralSubscription.subscriber = %(personid)s
-                UNION ALL
-                SELECT BugTask.id FROM BugTask, StructuralSubscription
-                WHERE
-                  BugTask.milestone = StructuralSubscription.milestone
-                  AND StructuralSubscription.subscriber = %(personid)s
-                UNION ALL
-                SELECT BugTask.id FROM BugTask, StructuralSubscription
-                WHERE
-                  BugTask.productseries = StructuralSubscription.productseries
-                  AND StructuralSubscription.subscriber = %(personid)s
-                UNION ALL
-                SELECT BugTask.id
-                FROM BugTask, StructuralSubscription, Product
-                WHERE
-                  BugTask.product = Product.id
-                  AND Product.project = StructuralSubscription.project
-                  AND StructuralSubscription.subscriber = %(personid)s
-                UNION ALL
-                SELECT BugTask.id FROM BugTask, StructuralSubscription
-                WHERE
-                  BugTask.distribution = StructuralSubscription.distribution
-                  AND StructuralSubscription.sourcepackagename is NULL
-                  AND StructuralSubscription.subscriber = %(personid)s)""" %
-                sqlvalues(personid=params.structural_subscriber))
-            extra_clauses.append(structural_subscriber_clause)
+            ssub_match_product = (
+                BugTask.productID ==
+                StructuralSubscription.productID)
+            ssub_match_productseries = (
+                BugTask.productseriesID ==
+                StructuralSubscription.productseriesID)
+            # Prevent circular import problems.
+            from lp.registry.model.product import Product
+            ssub_match_project = And(
+                Product.projectID ==
+                StructuralSubscription.projectID,
+                BugTask.product == Product.id)
+            ssub_match_distribution = (
+                BugTask.distributionID ==
+                StructuralSubscription.distributionID)
+            ssub_match_sourcepackagename = (
+                BugTask.sourcepackagenameID ==
+                StructuralSubscription.sourcepackagenameID)
+            ssub_match_null_sourcepackagename = (
+                StructuralSubscription.sourcepackagename == None)
+            ssub_match_distribution_with_optional_package = And(
+                ssub_match_distribution, Or(
+                    ssub_match_sourcepackagename,
+                    ssub_match_null_sourcepackagename))
+            ssub_match_distribution_series = (
+                BugTask.distroseriesID ==
+                StructuralSubscription.distroseriesID)
+            ssub_match_milestone = (
+                BugTask.milestoneID ==
+                StructuralSubscription.milestoneID)
+
+            join_clause = Or(
+                ssub_match_product,
+                ssub_match_productseries,
+                ssub_match_project,
+                ssub_match_distribution_with_optional_package,
+                ssub_match_distribution_series,
+                ssub_match_milestone)
+
+            join_tables.append(
+                (Product, LeftJoin(Product, BugTask.productID == Product.id)))
+            join_tables.append(
+                (StructuralSubscription,
+                 Join(StructuralSubscription, join_clause)))
+            extra_clauses.append(
+                'StructuralSubscription.subscriber = %s'
+                % sqlvalues(params.structural_subscriber))
+            has_duplicate_results = True
 
         if params.component:
             clauseTables += [SourcePackagePublishingHistory,
@@ -1928,7 +1939,9 @@
                 for decor in decorators:
                     obj = decor(obj)
                 return obj
-        return query, clauseTables, orderby_arg, decorator
+        return (
+            query, clauseTables, orderby_arg, decorator, join_tables,
+            has_duplicate_results)
 
     def _buildUpstreamClause(self, params):
         """Return an clause for returning upstream data if the data exists.
@@ -2156,101 +2169,137 @@
             ', '.join(tables), ' AND '.join(clauses))
         return clause
 
-    def search(self, params, *args, **kwargs):
-        """See `IBugTaskSet`.
-
-        :param _noprejoins: Private internal parameter to BugTaskSet which
-            disables all use of prejoins : consolidated from code paths that
-            claim they were inefficient and unwanted.
-        """
-        # Circular.
-        from lp.registry.model.product import Product
-        from lp.bugs.model.bug import Bug
-        _noprejoins = kwargs.get('_noprejoins', False)
+    def buildOrigin(self, join_tables, prejoin_tables, clauseTables):
+        """Build the parameter list for Store.using().
+
+        :param join_tables: A sequence of tables that should be joined
+            as returned by buildQuery(). Each element has the form
+            (table, join), where table is the table to join and join
+            is a Storm Join or LeftJoin instance.
+        :param prejoin_tables: A sequence of tables that should additionally
+            be joined. Each element has the form (table, join),
+            where table is the table to join and join is a Storm Join
+            or LeftJoin instance.
+        :param clauseTables: A sequence of tables that should appear in
+            the FROM clause of a query. The join condition is defined in
+            the WHERE clause.
+
+        Tables may appear simultaneously in join_tables, prejoin_tables
+        and in clauseTables. This method ensures that each table
+        appears exactly once in the returned sequence.
+        """
+        origin = [BugTask]
+        already_joined = set(origin)
+        for table, join in join_tables:
+            origin.append(join)
+            already_joined.add(table)
+        for table, join in prejoin_tables:
+            if table not in already_joined:
+                origin.append(join)
+                already_joined.add(table)
+        for table in clauseTables:
+            if table not in already_joined:
+                origin.append(table)
+        return origin
+
+    def _search(self, resultrow, prejoins, params, *args, **kw):
+        """Return a Storm result set for the given search parameters.
+
+        :param resultrow: The type of data returned by the query.
+        :param prejoins: A sequence of Storm SQL row instances which are
+            pre-joined.
+        :param params: A BugTaskSearchParams instance.
+        :param args: optional additional BugTaskSearchParams instances,
+        """
         store = IStore(BugTask)
-        query, clauseTables, orderby, bugtask_decorator = self.buildQuery(
-            params)
+        [query, clauseTables, orderby, bugtask_decorator, join_tables,
+        has_duplicate_results] = self.buildQuery(params)
         if len(args) == 0:
-            if _noprejoins:
-                resultset = store.find(BugTask,
-                    AutoTables(SQL("1=1"), clauseTables),
-                    query)
+            if has_duplicate_results:
+                origin = self.buildOrigin(join_tables, [], clauseTables)
+                outer_origin = self.buildOrigin([], prejoins, [])
+                subquery = Select(BugTask.id, where=SQL(query), tables=origin)
+                resultset = store.using(*outer_origin).find(
+                    resultrow, In(BugTask.id, subquery))
+            else:
+                origin = self.buildOrigin(join_tables, prejoins, clauseTables)
+                resultset = store.using(*origin).find(resultrow, query)
+            if prejoins:
+                decorator = lambda row: bugtask_decorator(row[0])
+            else:
                 decorator = bugtask_decorator
-            else:
-                tables = clauseTables + [Product, SourcePackageName]
-                origin = [
-                    BugTask,
-                    LeftJoin(Bug, BugTask.bug == Bug.id),
-                    LeftJoin(Product, BugTask.product == Product.id),
-                    LeftJoin(
-                        SourcePackageName,
-                        BugTask.sourcepackagename == SourcePackageName.id),
-                    ]
-                # NB: these may work with AutoTables, but its hard to tell,
-                # this way is known to work.
-                if BugNomination in tables:
-                    # The relation is already in query.
-                    origin.append(BugNomination)
-                if BugSubscription in tables:
-                    # The relation is already in query.
-                    origin.append(BugSubscription)
-                if SourcePackageRelease in tables:
-                    origin.append(SourcePackageRelease)
-                if SourcePackagePublishingHistory in tables:
-                    origin.append(SourcePackagePublishingHistory)
-                resultset = store.using(*origin).find(
-                    (BugTask, Product, SourcePackageName, Bug),
-                    AutoTables(SQL("1=1"), tables),
-                    query)
-                decorator=lambda row: bugtask_decorator(row[0])
+
             resultset.order_by(orderby)
             return DecoratedResultSet(resultset, result_decorator=decorator)
 
         bugtask_fti = SQL('BugTask.fti')
-        result = store.find((BugTask, bugtask_fti), query,
-                            AutoTables(SQL("1=1"), clauseTables))
+        inner_resultrow = (BugTask, bugtask_fti)
+        origin = self.buildOrigin(join_tables, [], clauseTables)
+        resultset = store.using(*origin).find(inner_resultrow, query)
+
         decorators = [bugtask_decorator]
         for arg in args:
-            query, clauseTables, dummy, decorator = self.buildQuery(arg)
-            result = result.union(
-                store.find((BugTask, bugtask_fti), query,
-                           AutoTables(SQL("1=1"), clauseTables)))
+            [query, clauseTables, ignore, decorator, join_tables,
+             has_duplicate_results] = self.buildQuery(arg)
+            origin = self.buildOrigin(join_tables, [], clauseTables)
+            next_result = store.using(*origin).find(inner_resultrow, query)
+            resultset = resultset.union(next_result)
             # NB: assumes the decorators are all compatible.
             # This may need revisiting if e.g. searches on behalf of different
             # users are combined.
             decorators.append(decorator)
 
-        def decorator(row):
+        def prejoin_decorator(row):
             bugtask = row[0]
             for decorator in decorators:
                 bugtask = decorator(bugtask)
             return bugtask
 
-        # Build up the joins.
-        # TODO: implement _noprejoins for this code path: as of 20100818 it
-        # has been silently disabled because clients of the API were setting
-        # prejoins=[] which had no effect; this TODO simply notes the reality
-        # already existing when it was added.
-        joins = Alias(result._get_select(), "BugTask")
-        joins = Join(joins, Bug, BugTask.bug == Bug.id)
-        joins = LeftJoin(joins, Product, BugTask.product == Product.id)
-        joins = LeftJoin(joins, SourcePackageName,
-                         BugTask.sourcepackagename == SourcePackageName.id)
-
-        result = store.using(joins).find(
-            (BugTask, Bug, Product, SourcePackageName))
+        def simple_decorator(bugtask):
+            for decorator in decorators:
+                bugtask = decorator(bugtask)
+            return bugtask
+
+        origin = [Alias(resultset._get_select(), "BugTask")]
+        if prejoins:
+            origin += [join for table, join in prejoins]
+            decorator = prejoin_decorator
+        else:
+            decorator = simple_decorator
+
+        result = store.using(*origin).find(resultrow)
         result.order_by(orderby)
         return DecoratedResultSet(result, result_decorator=decorator)
 
+    def search(self, params, *args, **kwargs):
+        """See `IBugTaskSet`.
+
+        :param _noprejoins: Private internal parameter to BugTaskSet which
+            disables all use of prejoins : consolidated from code paths that
+            claim they were inefficient and unwanted.
+        """
+        # Prevent circular import problems.
+        from lp.registry.model.product import Product
+        from lp.bugs.model.bug import Bug
+        _noprejoins = kwargs.get('_noprejoins', False)
+        if _noprejoins:
+            prejoins = []
+            resultrow = BugTask
+        else:
+            prejoins = [
+                (Bug, LeftJoin(Bug, BugTask.bug == Bug.id)),
+                (Product, LeftJoin(Product, BugTask.product == Product.id)),
+                (SourcePackageName,
+                 LeftJoin(
+                     SourcePackageName,
+                     BugTask.sourcepackagename == SourcePackageName.id)),
+                ]
+            resultrow = (BugTask, Bug, Product, SourcePackageName, )
+        return self._search(resultrow, prejoins, params, *args)
+
     def searchBugIds(self, params):
         """See `IBugTaskSet`."""
-        query, clauseTables, orderby, decorator = self.buildQuery(
-            params)
-        store = IStore(BugTask)
-        resultset = store.find(BugTask.bugID,
-            AutoTables(SQL("1=1"), clauseTables), query)
-        resultset.order_by(orderby)
-        return resultset
+        return self._search(BugTask.bugID, [], params).result_set
 
     def getAssignedMilestonesFromSearch(self, search_results):
         """See `IBugTaskSet`."""
@@ -2854,8 +2903,8 @@
 
         if recipients is not None:
             # We need to process subscriptions, so pull all the
-            # subscribes into the cache, then update recipients with
-            # the subscriptions.
+            # subscribers into the cache, then update recipients
+            # with the subscriptions.
             subscribers = list(subscribers)
             for subscription in subscriptions:
                 recipients.addStructuralSubscriber(

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2010-11-04 15:53:15 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2010-11-17 13:53:59 +0000
@@ -507,6 +507,23 @@
         self.assertSearchFinds(params, self.bugtasks[:1])
 
 
+class ProjectGroupAndDistributionTests:
+    """Tests which are useful for project groups and distributions."""
+
+    def setUpStructuralSubscriptions(self):
+        # Subscribe a user to the search target of this test and to
+        # another target.
+        raise NotImplementedError
+
+    def test_unique_results_for_multiple_structural_subscriptions(self):
+        # Searching for a subscriber who is more than once subscribed to a
+        # bug task returns this bug task only once.
+        subscriber = self.setUpStructuralSubscriptions()
+        params = self.getBugTaskSearchParams(
+            user=None, structural_subscriber=subscriber)
+        self.assertSearchFinds(params, self.bugtasks)
+
+
 class BugTargetTestBase:
     """A base class for the bug target mixin classes."""
 
@@ -628,7 +645,8 @@
             bugtask, self.searchtarget.product)
 
 
-class ProjectGroupTarget(BugTargetTestBase, BugTargetWithBugSuperVisor):
+class ProjectGroupTarget(BugTargetTestBase, BugTargetWithBugSuperVisor,
+                         ProjectGroupAndDistributionTests):
     """Use a project group as the bug target."""
 
     def setUp(self):
@@ -698,6 +716,15 @@
             'No bug task found for a product that is not the target of '
             'the main test bugtask.')
 
+    def setUpStructuralSubscriptions(self):
+        # See `ProjectGroupAndDistributionTests`.
+        subscriber = self.factory.makePerson()
+        self.subscribeToTarget(subscriber)
+        with person_logged_in(subscriber):
+            self.bugtasks[0].target.addSubscription(
+                subscriber, subscribed_by=subscriber)
+        return subscriber
+
 
 class MilestoneTarget(BugTargetTestBase):
     """Use a milestone as the bug target."""
@@ -731,7 +758,8 @@
 
 
 class DistributionTarget(BugTargetTestBase, ProductAndDistributionTests,
-                         BugTargetWithBugSuperVisor):
+                         BugTargetWithBugSuperVisor,
+                         ProjectGroupAndDistributionTests):
     """Use a distribution as the bug target."""
 
     def setUp(self):
@@ -753,6 +781,18 @@
         """See `ProductAndDistributionTests`."""
         return self.factory.makeDistroSeries(distribution=self.searchtarget)
 
+    def setUpStructuralSubscriptions(self):
+        # See `ProjectGroupAndDistributionTests`.
+        subscriber = self.factory.makePerson()
+        sourcepackage = self.factory.makeDistributionSourcePackage(
+            distribution=self.searchtarget)
+        self.bugtasks.append(self.factory.makeBugTask(target=sourcepackage))
+        self.subscribeToTarget(subscriber)
+        with person_logged_in(subscriber):
+            sourcepackage.addSubscription(
+                subscriber, subscribed_by=subscriber)
+        return subscriber
+
 
 class DistroseriesTarget(BugTargetTestBase):
     """Use a distro series as the bug target."""
@@ -838,7 +878,30 @@
     )
 
 
-class PreloadBugtaskTargets:
+class MultipleParams:
+    """A mixin class for tests with more than one search parameter object.
+
+    BugTaskSet.search() can be called with more than one
+    BugTaskSearchParams instances, while BugTaskSet.searchBugIds()
+    accepts exactly one instance.
+    """
+
+    def test_two_param_objects(self):
+        # We can pass more than one BugTaskSearchParams instance to
+        # BugTaskSet.search().
+        params1 = self.getBugTaskSearchParams(
+            user=None, status=BugTaskStatus.FIXCOMMITTED)
+        subscriber = self.factory.makePerson()
+        self.subscribeToTarget(subscriber)
+        params2 = self.getBugTaskSearchParams(
+            user=None, status=BugTaskStatus.NEW,
+            structural_subscriber=subscriber)
+        search_result = self.runSearch(params1, params2)
+        expected = self.resultValuesForBugtasks(self.bugtasks[1:])
+        self.assertEqual(expected, search_result)
+
+
+class PreloadBugtaskTargets(MultipleParams):
     """Preload bug targets during a BugTaskSet.search() query."""
 
     def setUp(self):
@@ -852,7 +915,7 @@
         return expected_bugtasks
 
 
-class NoPreloadBugtaskTargets:
+class NoPreloadBugtaskTargets(MultipleParams):
     """Do not preload bug targets during a BugTaskSet.search() query."""
 
     def setUp(self):