launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/proper-error-when-searchtasks-orderby into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #904703 in Launchpad itself: "KeyError when searchTask gets the wrong order_by arg"
  https://bugs.launchpad.net/launchpad/+bug/904703

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/proper-error-when-searchtasks-orderby/+merge/124609

Currently, if you pass the wrong order_by argument to searchTasks, it will raise an OOPS with a KeyError. Slightly restructure the code and make sure to raise a 400 Bad Request if the argument is wrong. I have also changed all occurances of ValueError in bugtasksearch to be more friendly to the API, but have subclassed ValueError so that existing browser code will cope.
-- 
https://code.launchpad.net/~stevenk/launchpad/proper-error-when-searchtasks-orderby/+merge/124609
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/proper-error-when-searchtasks-orderby into lp:launchpad.

=== modified file 'lib/lp/bugs/errors.py'
--- lib/lp/bugs/errors.py	2012-08-28 03:56:19 +0000
+++ lib/lp/bugs/errors.py	2012-09-17 07:05:37 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'BugSearchError',
     'InvalidDuplicateValue',
 ]
 
@@ -15,6 +16,11 @@
 from lp.app.validators import LaunchpadValidationError
 
 
+@error_status(httplib.BAD_REQUEST)
+class BugSearchError(ValueError):
+    """An error occured during searching for bugs."""
+
+
 @error_status(httplib.EXPECTATION_FAILED)
 class InvalidDuplicateValue(LaunchpadValidationError):
     """A bug cannot be set as the duplicate of another."""

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-08-08 05:43:18 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-09-17 07:05:37 +0000
@@ -41,6 +41,7 @@
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.blueprints.model.specification import Specification
 from lp.blueprints.model.specificationbug import SpecificationBug
+from lp.bugs.errors import BugSearchError
 from lp.bugs.interfaces.bugattachment import BugAttachmentType
 from lp.bugs.interfaces.bugnomination import BugNominationStatus
 from lp.bugs.interfaces.bugtask import (
@@ -383,7 +384,7 @@
                 'Excluding conjoined tasks is not currently supported '
                 'for milestone tags')
         if not params.milestone:
-            raise ValueError(
+            raise BugSearchError(
                 "BugTaskSearchParam.exclude_conjoined cannot be True if "
                 "BugTaskSearchParam.milestone is not set")
 
@@ -578,7 +579,7 @@
         elif params.distroseries:
             distroseries = params.distroseries
         if distroseries is None:
-            raise ValueError(
+            raise BugSearchError(
                 "Search by component requires a context with a "
                 "distribution or distroseries.")
 
@@ -827,13 +828,17 @@
         if isinstance(orderby_col, SQLConstant):
             orderby_arg.append(orderby_col)
             continue
+        desc_search = False
         if orderby_col.startswith("-"):
-            col, sort_joins = orderby_expression[orderby_col[1:]]
-            extra_joins.extend(sort_joins)
+            orderby_col = orderby_col[1:]
+            desc_search = True
+        if orderby_col not in orderby_expression:
+            raise BugSearchError("Unrecognized order_by: %r" % (orderby_col,))
+        col, sort_joins = orderby_expression[orderby_col]
+        extra_joins.extend(sort_joins)
+        if desc_search:
             order_clause = Desc(col)
         else:
-            col, sort_joins = orderby_expression[orderby_col]
-            extra_joins.extend(sort_joins)
             order_clause = col
         if col in unambiguous_cols:
             ambiguous = False
@@ -906,7 +911,7 @@
             status = any(*DB_INCOMPLETE_BUGTASK_STATUSES)
         return search_value_to_storm_where_condition(col, status)
     else:
-        raise ValueError('Unrecognized status value: %r' % (status,))
+        raise BugSearchError('Unrecognized status value: %r' % (status,))
 
 
 def _build_exclude_conjoined_clause(milestone):

=== modified file 'lib/lp/bugs/tests/test_searchtasks_webservice.py'
--- lib/lp/bugs/tests/test_searchtasks_webservice.py	2012-08-08 11:48:29 +0000
+++ lib/lp/bugs/tests/test_searchtasks_webservice.py	2012-09-17 07:05:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Webservice unit tests related to Launchpad Bugs."""
@@ -148,3 +148,26 @@
         # A non-matching search returns no results.
         response = self.search("devel", bug_id=0)
         self.assertEqual(len(response), 0)
+
+
+class TestOrderingSearchResults(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestOrderingSearchResults, self).setUp()
+        self.owner = self.factory.makePerson()
+        with person_logged_in(self.owner):
+            self.product = self.factory.makeProduct()
+        self.bug = self.factory.makeBugTask(target=self.product)
+        self.webservice = LaunchpadWebServiceCaller(
+            'launchpad-library', 'salgado-change-anything')
+
+    def test_search_with_wrong_orderby(self):
+        response = self.webservice.named_get(
+            '/%s' % self.product.name, 'searchTasks',
+            api_version='devel', order_by='date_created')
+        self.assertEqual(400, response.status)
+        self.assertRaisesWithContent(
+            ValueError, "Unrecognized order_by: u'date_created'",
+            response.jsonBody)