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

[Abel Deuring]

[r=gmb][bug=909318] bug fix: invalid SQL query when a bugtask search consisting of a UNION of two or more sub-.queries is ordered by a value that requires to join another table.

This branch fixes bug 909318: Can’t order bugs by reporter.

The cause is a bug in the SQL query generated by BugTaskSet._search()
when the *args passed to _search are not empty (i.e., when the query
is a UNION of several SELECTs), and when the ordering of the result
set requires to JOIN another table. In this case, the query looked
roughly like:

SELECT BugTask.* FROM (
   SELECT BugTask.* JOIN SortTable ON ... WHERE condition1
   UNION SELECT BugTask.* JOIN SortTable ON ... WHERE condition2
   UNION ...)
   ORDER BY SortTable.sort_column;

Joining the sort table in the different term of the UNION operator
is obviously nonsense.

The expression "JOIN SortTable ON ..." was added in
BugTaskSet.buildQuery() (hunk @@ -2368,9 +2368,6 @@ of the diff).

I moved this to BugTaskSet._search(): BugTaskSet._processOrderBy()
returns (orderby_arg, extra_joins), where extra_joins is a sequence
of tuples (join_table, join_expression), as can be passed to
BugTaskSet.buildOrigin(). So we can conveniently decide in _search()
to which SELECT the ordering related JOINs are added.

tests:

./bin/test bugs -vvt test_bugtask_search.*test_two_param_objects -vvt test_bugtask_search.*test_sort_by

Demo/QA: Visit the bug page of any person/team and sort the
bugs by assignee, reporter or milestone.

no lint

[Graham Binns (gmb)]

139 + def test_two_param_objects_sorting_needs_extra_join(self):

This needs a comment stating the expected behaviour you're testing for.

review: Approve (code)

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-12-30 06:14:56 +0000
+++ lib/lp/bugs/model/bugtask.py	2012-01-04 08:48:29 +0000
@@ -2368,9 +2368,6 @@
                 "BugTask.datecreated > %s" % (
                     sqlvalues(params.created_since,)))
 
-        orderby_arg, extra_joins = self._processOrderBy(params)
-        join_tables.extend(extra_joins)
-
         query = " AND ".join(extra_clauses)
 
         if not decorators:
@@ -2386,7 +2383,7 @@
         else:
             with_clause = None
         return (
-            query, clauseTables, orderby_arg, decorator, join_tables,
+            query, clauseTables, decorator, join_tables,
             has_duplicate_results, with_clause)
 
     def buildUpstreamClause(self, params):
@@ -2644,7 +2641,8 @@
                     SpecificationBug.specification %s)
                 """ % search_value_to_where_condition(linked_blueprints)
 
-    def buildOrigin(self, join_tables, prejoin_tables, clauseTables):
+    def buildOrigin(self, join_tables, prejoin_tables, clauseTables,
+                    start_with=BugTask):
         """Build the parameter list for Store.using().
 
         :param join_tables: A sequence of tables that should be joined
@@ -2663,7 +2661,7 @@
         and in clauseTables. This method ensures that each table
         appears exactly once in the returned sequence.
         """
-        origin = [BugTask]
+        origin = [start_with]
         already_joined = set(origin)
         for table, join in join_tables:
             if table is None or table not in already_joined:
@@ -2691,26 +2689,29 @@
         :param args: optional additional BugTaskSearchParams instances,
         """
         orig_store = store = IStore(BugTask)
-        [query, clauseTables, orderby, bugtask_decorator, join_tables,
+        [query, clauseTables, bugtask_decorator, join_tables,
         has_duplicate_results, with_clause] = self.buildQuery(params)
         if with_clause:
             store = store.with_(with_clause)
+        orderby_expression, orderby_joins = self._processOrderBy(params)
         if len(args) == 0:
             if has_duplicate_results:
                 origin = self.buildOrigin(join_tables, [], clauseTables)
-                outer_origin = self.buildOrigin([], prejoins, [])
+                outer_origin = self.buildOrigin(
+                     orderby_joins, 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)
+                origin = self.buildOrigin(
+                    join_tables + orderby_joins, prejoins, clauseTables)
                 resultset = store.using(*origin).find(resultrow, query)
             if prejoins:
                 decorator = lambda row: bugtask_decorator(row[0])
             else:
                 decorator = bugtask_decorator
 
-            resultset.order_by(orderby)
+            resultset.order_by(orderby_expression)
             return DecoratedResultSet(resultset, result_decorator=decorator,
                 pre_iter_hook=pre_iter_hook)
 
@@ -2720,7 +2721,7 @@
 
         decorators = [bugtask_decorator]
         for arg in args:
-            [query, clauseTables, ignore, decorator, join_tables,
+            [query, clauseTables, decorator, join_tables,
              has_duplicate_results, with_clause] = self.buildQuery(arg)
             origin = self.buildOrigin(join_tables, [], clauseTables)
             localstore = store
@@ -2745,15 +2746,16 @@
                 bugtask = decorator(bugtask)
             return bugtask
 
-        origin = [Alias(resultset._get_select(), "BugTask")]
+        origin = self.buildOrigin(
+            orderby_joins, prejoins, [],
+            start_with=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)
+        result.order_by(orderby_expression)
         return DecoratedResultSet(result, result_decorator=decorator,
             pre_iter_hook=pre_iter_hook)
 

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2012-01-04 08:48:29 +0000
@@ -1339,20 +1339,39 @@
     accepts exactly one instance.
     """
 
+    def setUpTwoSearchParams(self, orderby=None):
+        # Prepare the test data for the tests in this class.
+        params1 = self.getBugTaskSearchParams(
+            user=None, status=BugTaskStatus.FIXCOMMITTED, orderby=orderby)
+        subscriber = self.factory.makePerson()
+        self.subscribeToTarget(subscriber)
+        params2 = self.getBugTaskSearchParams(
+            user=None, status=BugTaskStatus.NEW,
+            structural_subscriber=subscriber, orderby=orderby)
+        return params1, params2
+
     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)
+        params1, params2 = self.setUpTwoSearchParams()
         search_result = self.runSearch(params1, params2)
         expected = self.resultValuesForBugtasks(self.bugtasks[1:])
         self.assertEqual(expected, search_result)
 
+    def test_two_param_objects_sorting_needs_extra_join(self):
+        # If result ordering needs an extra join, the join
+        # is added to the union of the result sets for the two
+        # BugTaskSearchParams instances.
+        params1, params2 = self.setUpTwoSearchParams(orderby='reporter')
+        search_result = self.runSearch(params1, params2)
+
+        def sortkey(bugtask):
+            return bugtask.owner.name
+
+        expected_bugtasks = sorted(self.bugtasks[1:], key=sortkey)
+        expected = self.resultValuesForBugtasks(expected_bugtasks)
+        self.assertEqual(expected, search_result)
+
 
 class PreloadBugtaskTargets(MultipleParams):
     """Preload bug targets during a BugTaskSet.search() query."""