Merge lp:~wgrant/launchpad/bugtaskflat-search-3 into lp:launchpad

[William Grant]

BugTaskFlat is now used for sorting and filtering when the flag is enabled.

This branch basically completes the initial BugTaskFlat search implementation, porting filtering and sorting to fields on BugTaskFlat where possible. This lets us eliminate the Bug and BugTask joins from most searches.

The only radical difference between the two is the private bug filter. It's traditionally done by joining against BugSubscription, but with BugTaskFlat it's just a matter of checking intersection of the user's teams with a denormalised array. There's only one functional difference: assignees can no longer see their bugs unless they have an explicit grant, which isn't ensured by the code yet. But this is a corner case that was only added recently, so it shouldn't be a problem for the short time it's inconsistent.

[Ian Booth (wallyworld)]

Looks ok, although I am not intimately familiar with the other branches in this pipe.
As discussed, we need a plan to ensure assignees can see their own bugs since the new work removes that "feature".

review: Approve

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2012-04-17 07:54:24 +0000
+++ lib/lp/bugs/browser/bugtask.py	2012-04-19 04:04:19 +0000
@@ -217,7 +217,7 @@
 from lp.bugs.interfaces.bugwatch import BugWatchActivityStatus
 from lp.bugs.interfaces.cve import ICveSet
 from lp.bugs.interfaces.malone import IMaloneApplication
-from lp.bugs.model.bugtasksearch import orderby_expression
+from lp.bugs.model.bugtasksearch import unflat_orderby_expression
 from lp.code.interfaces.branchcollection import IAllBranches
 from lp.layers import FeedsLayer
 from lp.registry.enums import InformationType
@@ -2802,7 +2802,7 @@
                 orderby_col = orderby_col[1:]
 
             try:
-                orderby_expression[orderby_col]
+                unflat_orderby_expression[orderby_col]
             except KeyError:
                 raise UnexpectedFormData(
                     "Unknown sort column '%s'" % orderby_col)

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-04-17 22:56:29 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-04-19 04:04:19 +0000
@@ -52,7 +52,7 @@
     IBugTask,
     IBugTaskSet,
     )
-from lp.bugs.model.bugtasksearch import orderby_expression
+from lp.bugs.model.bugtasksearch import unflat_orderby_expression
 from lp.layers import (
     FeedsLayer,
     setFirstLayer,
@@ -2325,7 +2325,7 @@
         cache = IJSONRequestCache(view.request)
         json_sort_keys = cache.objects['sort_keys']
         json_sort_keys = set(key[0] for key in json_sort_keys)
-        valid_keys = set(orderby_expression.keys())
+        valid_keys = set(unflat_orderby_expression.keys())
         self.assertEqual(
             valid_keys, json_sort_keys,
             "Existing sort order values not available in JSON cache: %r; "

=== modified file 'lib/lp/bugs/model/bugtaskflat.py'
--- lib/lp/bugs/model/bugtaskflat.py	2012-04-16 08:12:26 +0000
+++ lib/lp/bugs/model/bugtaskflat.py	2012-04-19 04:04:19 +0000
@@ -2,6 +2,8 @@
     Bool,
     DateTime,
     Int,
+    Reference,
+    Storm,
     )
 
 from lp.registry.enums import InformationType
@@ -13,26 +15,39 @@
     )
 
 
-class BugTaskFlat(object):
+class BugTaskFlat(Storm):
 
     __storm_table__ = 'BugTaskFlat'
 
     bugtask_id = Int(name='bugtask', primary=True)
+    bugtask = Reference(bugtask_id, 'BugTask.id')
     bug_id = Int(name='bug')
+    bug = Reference(bug_id, 'Bug.id')
     datecreated = DateTime()
     duplicateof_id = Int(name='duplicateof')
+    duplicateof = Reference(duplicateof_id, 'Bug.id')
     bug_owner_id = Int(name='bug_owner')
+    bug_owner = Reference(bug_owner_id, 'Person.id')
     information_type = EnumCol(enum=InformationType)
     date_last_updated = DateTime()
     heat = Int()
     product_id = Int(name='product')
+    product = Reference(product_id, 'Product.id')
     productseries_id = Int(name='productseries')
+    productseries = Reference(productseries_id, 'ProductSeries.id')
     distribution_id = Int(name='distribution')
+    distribution = Reference(distribution_id, 'Distribution.id')
     distroseries_id = Int(name='distroseries')
+    distroseries = Reference(distroseries_id, 'DistroSeries.id')
     sourcepackagename_id = Int(name='sourcepackagename')
+    sourcepackagename = Reference(
+        sourcepackagename_id, 'SourcePackageName.id')
     status = EnumCol(schema=(BugTaskStatus, BugTaskStatusSearch))
     importance = EnumCol(schema=BugTaskImportance)
     assignee_id = Int(name='assignee')
+    assignee = Reference(assignee_id, 'Person.id')
     milestone_id = Int(name='milestone')
+    milestone = Reference(milestone_id, 'Milestone.id')
     owner_id = Int(name='owner')
+    owner = Reference(owner_id, 'Person.id')
     active = Bool()

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-04-18 02:10:59 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-04-19 04:04:19 +0000
@@ -5,7 +5,7 @@
 
 __all__ = [
     'get_bug_privacy_filter',
-    'orderby_expression',
+    'unflat_orderby_expression',
     'search_bugs',
     ]
 
@@ -62,6 +62,7 @@
 from lp.bugs.model.bugtask import BugTask
 from lp.bugs.model.bugtaskflat import BugTaskFlat
 from lp.bugs.model.structuralsubscription import StructuralSubscription
+from lp.registry.enums import PUBLIC_INFORMATION_TYPES
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.milestone import IProjectGroupMilestone
@@ -75,10 +76,7 @@
 from lp.services.database.bulk import load
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.lpstorm import IStore
-from lp.services.database.sqlbase import (
-    quote,
-    sqlvalues,
-    )
+from lp.services.database.sqlbase import sqlvalues
 from lp.services.database.stormexpr import (
     Array,
     NullCount,
@@ -97,7 +95,7 @@
 
 # This abstracts most of the columns involved in search so we can switch
 # to/from BugTaskFlat easily.
-cols = {
+unflat_cols = {
     'Bug.id': Bug.id,
     'Bug.duplicateof': Bug.duplicateof,
     'Bug.owner': Bug.owner,
@@ -125,11 +123,39 @@
     'BugTask._status': BugTask._status,
     }
 
+flat_cols = {
+    'Bug.id': BugTaskFlat.bug_id,
+    'Bug.duplicateof': BugTaskFlat.duplicateof,
+    'Bug.owner': BugTaskFlat.bug_owner,
+    'Bug.date_last_updated': BugTaskFlat.date_last_updated,
+    'BugTask.id': BugTaskFlat.bugtask_id,
+    'BugTask.bug': BugTaskFlat.bug,
+    'BugTask.bugID': BugTaskFlat.bug_id,
+    'BugTask.importance': BugTaskFlat.importance,
+    'BugTask.product': BugTaskFlat.product,
+    'BugTask.productID': BugTaskFlat.product_id,
+    'BugTask.productseries': BugTaskFlat.productseries,
+    'BugTask.productseriesID': BugTaskFlat.productseries_id,
+    'BugTask.distribution': BugTaskFlat.distribution,
+    'BugTask.distributionID': BugTaskFlat.distribution_id,
+    'BugTask.distroseries': BugTaskFlat.distroseries,
+    'BugTask.distroseriesID': BugTaskFlat.distroseries_id,
+    'BugTask.sourcepackagename': BugTaskFlat.sourcepackagename,
+    'BugTask.sourcepackagenameID': BugTaskFlat.sourcepackagename_id,
+    'BugTask.milestone': BugTaskFlat.milestone,
+    'BugTask.milestoneID': BugTaskFlat.milestone_id,
+    'BugTask.assignee': BugTaskFlat.assignee,
+    'BugTask.owner': BugTaskFlat.owner,
+    'BugTask.date_closed': BugTask.date_closed,
+    'BugTask.datecreated': BugTaskFlat.datecreated,
+    'BugTask._status': BugTaskFlat.status,
+    }
+
 
 bug_join = (Bug, Join(Bug, BugTask.bug == Bug.id))
 Assignee = ClassAlias(Person)
 Reporter = ClassAlias(Person)
-orderby_expression = {
+unflat_orderby_expression = {
     "task": (BugTask.id, []),
     "id": (BugTask.bugID, []),
     "importance": (BugTask.importance, []),
@@ -206,6 +232,83 @@
         ),
     }
 
+flat_bug_join = (Bug, Join(Bug, Bug.id == BugTaskFlat.bug_id))
+flat_bugtask_join = (
+    BugTask, Join(BugTask, BugTask.id == BugTaskFlat.bugtask_id))
+flat_orderby_expression = {
+    "task": (BugTaskFlat.bugtask_id, []),
+    "id": (BugTaskFlat.bug_id, []),
+    "importance": (BugTaskFlat.importance, []),
+    # TODO: sort by their name?
+    "assignee": (
+        Assignee.name,
+        [
+            (Assignee,
+                LeftJoin(Assignee, BugTaskFlat.assignee == Assignee.id))
+            ]),
+    "targetname": (BugTask.targetnamecache, [flat_bugtask_join]),
+    "status": (BugTaskFlat.status, []),
+    "title": (Bug.title, [flat_bug_join]),
+    "milestone": (BugTaskFlat.milestone_id, []),
+    "dateassigned": (BugTask.date_assigned, [flat_bugtask_join]),
+    "datecreated": (BugTaskFlat.datecreated, []),
+    "date_last_updated": (BugTaskFlat.date_last_updated, []),
+    "date_closed": (BugTask.date_closed, [flat_bugtask_join]),
+    "number_of_duplicates": (Bug.number_of_duplicates, [flat_bug_join]),
+    "message_count": (Bug.message_count, [flat_bug_join]),
+    "users_affected_count": (Bug.users_affected_count, [flat_bug_join]),
+    "heat": (BugTaskFlat.heat, []),
+    "latest_patch_uploaded": (Bug.latest_patch_uploaded, [flat_bug_join]),
+    "milestone_name": (
+        Milestone.name,
+        [
+            (Milestone,
+                LeftJoin(Milestone,
+                        BugTaskFlat.milestone_id == Milestone.id))
+            ]),
+    "reporter": (
+        Reporter.name,
+        [
+            (Reporter, Join(Reporter, BugTaskFlat.bug_owner == Reporter.id))
+            ]),
+    "tag": (
+        BugTag.tag,
+        [
+            (BugTag,
+                LeftJoin(
+                    BugTag,
+                    BugTag.bug == BugTaskFlat.bug_id and
+                    # We want at most one tag per bug. Select the
+                    # tag that comes first in alphabetic order.
+                    BugTag.id == Select(
+                        BugTag.id, tables=[BugTag],
+                        where=(BugTag.bugID == BugTaskFlat.bug_id),
+                        order_by=BugTag.tag, limit=1))),
+            ]
+        ),
+    "specification": (
+        Specification.name,
+        [
+            (Specification,
+                LeftJoin(
+                    Specification,
+                    # We want at most one specification per bug.
+                    # Select the specification that comes first
+                    # in alphabetic order.
+                    Specification.id == Select(
+                        Specification.id,
+                        tables=[
+                            SpecificationBug,
+                            Join(
+                                Specification,
+                                Specification.id ==
+                                    SpecificationBug.specificationID)],
+                        where=(SpecificationBug.bugID == BugTaskFlat.bug_id),
+                        order_by=Specification.name, limit=1))),
+            ]
+        ),
+    }
+
 
 def search_value_to_storm_where_condition(comp, search_value):
     """Convert a search value to a Storm WHERE condition."""
@@ -362,14 +465,12 @@
         decorator to call on each returned row.
     """
     params = _require_params(params)
+    cols = flat_cols if use_flat else unflat_cols
 
     if use_flat:
         extra_clauses = []
         clauseTables = []
-        join_tables = [
-            (Bug, Join(Bug, Bug.id == BugTaskFlat.bug_id)),
-            (BugTask, Join(BugTask, BugTask.id == BugTaskFlat.bugtask_id)),
-            ]
+        join_tables = []
     else:
         extra_clauses = [Bug.id == BugTask.bugID]
         clauseTables = [BugTask, Bug]
@@ -509,10 +610,12 @@
                             BugAttachment.type, params.attachmenttype))))
 
     if params.searchtext:
-        extra_clauses.append(_build_search_text_clause(params))
+        extra_clauses.append(_build_search_text_clause(
+            params, use_flat=use_flat))
 
     if params.fast_searchtext:
-        extra_clauses.append(_build_search_text_clause(params, fast=True))
+        extra_clauses.append(_build_search_text_clause(
+            params, fast=True, use_flat=use_flat))
 
     if params.subscriber is not None:
         clauseTables.append(BugSubscription)
@@ -742,7 +845,8 @@
             extra_clauses.append(
                 Milestone.dateexpected <= dateexpected_before)
 
-    clause, decorator = _get_bug_privacy_filter_with_decorator(params.user)
+    clause, decorator = _get_bug_privacy_filter_with_decorator(
+        params.user, use_flat=use_flat)
     if clause:
         extra_clauses.append(SQL(clause))
         decorators.append(decorator)
@@ -821,15 +925,6 @@
     # decide whether we need to add the BugTask.bug or BugTask.id
     # columns to make the sort consistent over runs -- which is good
     # for the user and essential for the test suite.
-    unambiguous_cols = set([
-        Bug.date_last_updated,
-        Bug.datecreated,
-        Bug.id,
-        BugTask.bugID,
-        BugTask.date_assigned,
-        BugTask.datecreated,
-        BugTask.id,
-        ])
     # Bug ID is unique within bugs on a product or source package.
     if (params.product or
         (params.distribution and params.sourcepackagename) or
@@ -838,8 +933,34 @@
     else:
         in_unique_context = False
 
-    if in_unique_context:
-        unambiguous_cols.add(BugTask.bug)
+    if use_flat:
+        orderby_expression = flat_orderby_expression
+        unambiguous_cols = set([
+            BugTaskFlat.date_last_updated,
+            BugTaskFlat.datecreated,
+            BugTaskFlat.bugtask_id,
+            Bug.datecreated,
+            BugTask.date_assigned,
+            ])
+        if in_unique_context:
+            unambiguous_cols.add(BugTaskFlat.bug)
+    else:
+        orderby_expression = unflat_orderby_expression
+        # Bug.id and BugTask.bugID shouldn't really be here; they're
+        # ambiguous in a distribution or distroseries context. They're
+        # omitted from the new BugTaskFlat path, but kept in the legacy
+        # code in case it affects index selection.
+        unambiguous_cols = set([
+            Bug.date_last_updated,
+            Bug.datecreated,
+            Bug.id,
+            BugTask.bugID,
+            BugTask.date_assigned,
+            BugTask.datecreated,
+            BugTask.id,
+            ])
+        if in_unique_context:
+            unambiguous_cols.add(BugTask.bug)
 
     # Translate orderby keys into corresponding Table.attribute
     # strings.
@@ -882,10 +1003,16 @@
         orderby_arg.append(order_clause)
 
     if ambiguous:
-        if in_unique_context:
-            orderby_arg.append(BugTask.bugID)
+        if use_flat:
+            if in_unique_context:
+                orderby_arg.append(BugTaskFlat.bug_id)
+            else:
+                orderby_arg.append(BugTaskFlat.bugtask_id)
         else:
-            orderby_arg.append(BugTask.id)
+            if in_unique_context:
+                orderby_arg.append(BugTask.bugID)
+            else:
+                orderby_arg.append(BugTask.id)
 
     return tuple(orderby_arg), extra_joins
 
@@ -899,7 +1026,7 @@
     return params
 
 
-def _build_search_text_clause(params, fast=False):
+def _build_search_text_clause(params, fast=False, use_flat=False):
     """Build the clause for searchtext."""
     if fast:
         assert params.searchtext is None, (
@@ -910,12 +1037,14 @@
             'Cannot use fast_searchtext at the same time as searchtext.')
         searchtext = params.searchtext
 
+    col = 'BugTaskFlat.fti' if use_flat else 'Bug.fti'
+
     if params.orderby is None:
         # Unordered search results aren't useful, so sort by relevance
         # instead.
-        params.orderby = [SQL("-rank(Bug.fti, ftq(?))", params=(searchtext,))]
+        params.orderby = [SQL("-rank(%s, ftq(?))" % col, params=(searchtext,))]
 
-    return SQL("Bug.fti @@ ftq(?)", params=(searchtext,))
+    return SQL("%s @@ ftq(?)" % col, params=(searchtext,))
 
 
 def _build_status_clause(col, status):
@@ -1428,7 +1557,8 @@
     return cache_user_can_view_bug
 
 
-def _get_bug_privacy_filter_with_decorator(user, private_only=False):
+def _get_bug_privacy_filter_with_decorator(user, private_only=False,
+                                           use_flat=False):
     """Return a SQL filter to limit returned bug tasks.
 
     :param user: The user whose visible bugs will be filtered.
@@ -1438,38 +1568,48 @@
     :return: A SQL filter, a decorator to cache visibility in a resultset that
         returns BugTask objects.
     """
+    if use_flat:
+        public_bug_filter = (
+            'BugTaskFlat.information_type IN %s'
+            % sqlvalues(PUBLIC_INFORMATION_TYPES))
+    else:
+        public_bug_filter = 'Bug.private IS FALSE'
+
     if user is None:
-        return "Bug.private IS FALSE", _nocache_bug_decorator
+        return public_bug_filter, _nocache_bug_decorator
+
     admin_team = getUtility(ILaunchpadCelebrities).admin
     if user.inTeam(admin_team):
         return "", _nocache_bug_decorator
 
-    public_bug_filter = ''
+    if use_flat:
+        query = ("""
+            BugTaskFlat.access_grants &&
+            (SELECT array_agg(team) FROM teamparticipation WHERE person = %d)
+            """ % user.id)
+    else:
+        # A subselect is used here because joining through
+        # TeamParticipation is only relevant to the "user-aware"
+        # part of the WHERE condition (i.e. the bit below.) The
+        # other half of this condition (see code above) does not
+        # use TeamParticipation at all.
+        query = ("""
+            EXISTS (
+                WITH teams AS (
+                    SELECT team from TeamParticipation
+                    WHERE person = %d
+                )
+                SELECT BugSubscription.bug
+                FROM BugSubscription
+                WHERE BugSubscription.person IN (SELECT team FROM teams) AND
+                    BugSubscription.bug = Bug.id
+                UNION ALL
+                SELECT BugTask.bug
+                FROM BugTask
+                WHERE BugTask.assignee IN (SELECT team FROM teams) AND
+                    BugTask.bug = Bug.id
+                )
+            """ % user.id)
     if not private_only:
-        public_bug_filter = 'Bug.private IS FALSE OR'
-
-    # A subselect is used here because joining through
-    # TeamParticipation is only relevant to the "user-aware"
-    # part of the WHERE condition (i.e. the bit below.) The
-    # other half of this condition (see code above) does not
-    # use TeamParticipation at all.
-    query = """
-        (%(public_bug_filter)s EXISTS (
-            WITH teams AS (
-                SELECT team from TeamParticipation
-                WHERE person = %(personid)s
-            )
-            SELECT BugSubscription.bug
-            FROM BugSubscription
-            WHERE BugSubscription.person IN (SELECT team FROM teams) AND
-                BugSubscription.bug = Bug.id
-            UNION ALL
-            SELECT BugTask.bug
-            FROM BugTask
-            WHERE BugTask.assignee IN (SELECT team FROM teams) AND
-                BugTask.bug = Bug.id
-                ))
-        """ % dict(
-                personid=quote(user.id),
-                public_bug_filter=public_bug_filter)
-    return query, _make_cache_user_can_view_bug(user)
+        query = '%s OR %s' % (public_bug_filter, query)
+    return '(%s)' % query, _make_cache_user_can_view_bug(user)

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2012-04-17 23:50:01 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2012-04-19 04:04:19 +0000
@@ -121,18 +121,6 @@
         params = self.getBugTaskSearchParams(user=admin)
         self.assertSearchFinds(params, self.bugtasks)
 
-    def test_private_bug_in_search_result_assignees(self):
-        # Private bugs are included in search results for the assignee.
-        with person_logged_in(self.owner):
-            self.bugtasks[-1].bug.setPrivate(True, self.owner)
-        bugtask = self.bugtasks[-1]
-        user = self.factory.makePerson()
-        admin = getUtility(IPersonSet).getByEmail('[e-mail]')
-        with person_logged_in(admin):
-            bugtask.transitionToAssignee(user)
-        params = self.getBugTaskSearchParams(user=user)
-        self.assertSearchFinds(params, self.bugtasks)
-
     def test_search_by_bug_reporter(self):
         # Search results can be limited to bugs filed by a given person.
         bugtask = self.bugtasks[0]
@@ -1538,6 +1526,22 @@
             FeatureFixture({'bugs.bugtaskflat.search.enabled': 'on'}))
 
 
+class UsingLegacy:
+    """Use Bug and BugTask directly for searching."""
+
+    def test_private_bug_in_search_result_assignees(self):
+        # Private bugs are included in search results for the assignee.
+        with person_logged_in(self.owner):
+            self.bugtasks[-1].bug.setPrivate(True, self.owner)
+        bugtask = self.bugtasks[-1]
+        user = self.factory.makePerson()
+        admin = getUtility(IPersonSet).getByEmail('[e-mail]')
+        with person_logged_in(admin):
+            bugtask.transitionToAssignee(user)
+        params = self.getBugTaskSearchParams(user=user)
+        self.assertSearchFinds(params, self.bugtasks)
+
+
 class TestMilestoneDueDateFiltering(TestCaseWithFactory):
 
     layer = LaunchpadFunctionalLayer
@@ -1573,14 +1577,13 @@
     for bug_target_search_type_class in (
         PreloadBugtaskTargets, NoPreloadBugtaskTargets, QueryBugIDs):
         for target_mixin in bug_targets_mixins:
-            for feature_mixin in (None, UsingFlat):
+            for feature_mixin in (UsingLegacy, UsingFlat):
                 class_name = 'Test%s%s%s' % (
                     bug_target_search_type_class.__name__,
                     target_mixin.__name__,
-                    feature_mixin.__name__ if feature_mixin else '')
-                mixins = [target_mixin, bug_target_search_type_class]
-                if feature_mixin:
-                    mixins.append(feature_mixin)
+                    feature_mixin.__name__)
+                mixins = [
+                    target_mixin, bug_target_search_type_class, feature_mixin]
                 class_bases = (
                     tuple(mixins) + (SearchTestBase, TestCaseWithFactory))
                 # Dynamically build a test class from the target mixin class,