Merge lp:~stevenk/launchpad/proper-error-when-searchtasks-orderby into lp:launchpad

[Steve Kowalik]

Make searchTasks more friendly to the API by returning 400 Bad Request rather than ValueError/KeyError and OOPSing.

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.

[William Grant (wgrant)]

I'd hope the test could be moved into an existing class, but otherwise good.

review: Approve (code)

=== 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-18 04:33:20 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'InvalidDuplicateValue',
+    'InvalidSearchParameters',
 ]
 
 import httplib
@@ -18,3 +19,8 @@
 @error_status(httplib.EXPECTATION_FAILED)
 class InvalidDuplicateValue(LaunchpadValidationError):
     """A bug cannot be set as the duplicate of another."""
+
+
+@error_status(httplib.BAD_REQUEST)
+class InvalidSearchParameters(ValueError):
+    """Invalid search parameters were passed to searchTasks."""

=== 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-18 04:33:20 +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 InvalidSearchParameters
 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 InvalidSearchParameters(
                 "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 InvalidSearchParameters(
                 "Search by component requires a context with a "
                 "distribution or distroseries.")
 
@@ -827,13 +828,18 @@
         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 InvalidSearchParameters(
+                "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 +912,8 @@
             status = any(*DB_INCOMPLETE_BUGTASK_STATUSES)
         return search_value_to_storm_where_condition(col, status)
     else:
-        raise ValueError('Unrecognized status value: %r' % (status,))
+        raise InvalidSearchParameters(
+            'Unrecognized status value: %r' % (status,))
 
 
 def _build_exclude_conjoined_clause(milestone):

=== modified file 'lib/lp/bugs/model/tests/test_bugtasksearch.py'
--- lib/lp/bugs/model/tests/test_bugtasksearch.py	2012-08-24 05:09:51 +0000
+++ lib/lp/bugs/model/tests/test_bugtasksearch.py	2012-09-18 04:33:20 +0000
@@ -19,6 +19,7 @@
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.interfaces.services import IService
+from lp.bugs.errors import InvalidSearchParameters
 from lp.bugs.interfaces.bugattachment import BugAttachmentType
 from lp.bugs.interfaces.bugtarget import IBugTarget
 from lp.bugs.interfaces.bugtask import (
@@ -1803,13 +1804,13 @@
     def test_all_query(self):
         # Since status is single-valued, asking for "all" statuses in a set
         # doesn't make any sense.
-        with ExpectedException(ValueError):
+        with ExpectedException(InvalidSearchParameters):
             self.searchClause(
                 all(BugTaskStatus.NEW, BugTaskStatus.INCOMPLETE))
 
     def test_bad_value(self):
         # If an unrecognized status is provided then an error is raised.
-        with ExpectedException(ValueError):
+        with ExpectedException(InvalidSearchParameters):
             self.searchClause('this-is-not-a-status')
 
 

=== 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-18 04:33:20 +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."""
@@ -41,17 +41,20 @@
         self.assertEqual(response['total_size'], 1)
 
 
-class TestLinkedBlueprintsParameter(TestCaseWithFactory):
-    """Tests for the linked_blueprints parameter."""
+class TestProductSearchTasks(TestCaseWithFactory):
+    """Tests for the information_type, linked_blueprints and order_by
+    parameters."""
 
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
-        super(TestLinkedBlueprintsParameter, self).setUp()
+        super(TestProductSearchTasks, 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.bug = self.factory.makeBug(
+            target=self.product,
+            information_type=InformationType.PRIVATESECURITY)
         self.webservice = LaunchpadWebServiceCaller(
             'launchpad-library', 'salgado-change-anything')
 
@@ -83,28 +86,6 @@
         # thus no error is returned when we pass rubbish.
         self.search("beta", linked_blueprints="Teabags!")
 
-
-class TestSearchByInformationType(TestCaseWithFactory):
-    """Tests for the information_type parameter."""
-
-    layer = DatabaseFunctionalLayer
-
-    def setUp(self):
-        super(TestSearchByInformationType, self).setUp()
-        self.owner = self.factory.makePerson()
-        with person_logged_in(self.owner):
-            self.product = self.factory.makeProduct()
-        self.bug = self.factory.makeBug(
-            target=self.product,
-            information_type=InformationType.PRIVATESECURITY)
-        self.webservice = LaunchpadWebServiceCaller(
-            'launchpad-library', 'salgado-change-anything')
-
-    def search(self, api_version, **kwargs):
-        return self.webservice.named_get(
-            '/%s' % self.product.name, 'searchTasks',
-            api_version=api_version, **kwargs).jsonBody()
-
     def test_search_returns_results(self):
         # A matching search returns results.
         response = self.search(
@@ -116,6 +97,16 @@
         response = self.search("devel", information_type="Private")
         self.assertEqual(response['total_size'], 0)
 
+    def test_search_with_wrong_orderby(self):
+        # Calling searchTasks() with a wrong order_by is a Bad Request.
+        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)
+
 
 class TestGetBugData(TestCaseWithFactory):
     """Tests for the /bugs getBugData operation."""